Lazy initialize checkpoint tracker bit sets by jasontedor · Pull Request #27179 · elastic/elasticsearch

jasontedor · 2017-10-30T20:53:34Z

This local checkpoint tracker uses collections of bit sets to track which sequence numbers are complete, eventually removing these bit sets when the local checkpoint advances. However, these bit sets were eagerly allocated so that if a sequence number far ahead of the checkpoint was marked as completed, all bit sets between the "last" bit set and the bit set needed to track the marked sequence number were allocated. If this sequence number was too far ahead, the memory requirements could be excessive. This commit opts for a different strategy for holding on to these bit sets and enables them to be lazily allocated.

Relates #10708

This local checkpoint tracker uses collections of bit sets to track which sequence numbers are complete, eventually removing these bit sets when the local checkpoint advances. However, these bit sets were eagerly allocated so that if a sequence number far ahead of the checkpoint was marked as completed, all bit sets between the "last" bit set and the bit set needed to track the marked sequence number were allocated. If this sequence number was too far ahead, the memory requirements could be excessive. This commit opts for a different strategy for holding on to these bit sets and enables them to be lazily allocated.

bleskes

LGTM. Left some nits.

bleskes · 2017-10-30T22:01:25Z

core/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java

+            long bitArrayKey = getBitArrayKey(checkpoint);
+            FixedBitSet current = processedSeqNo.get(bitArrayKey);
+            if (current == null) {
+                // the bit set corresponding to the checkpoint has already been removed, set ourselves up for the next bit set


when can this happen? it seems we only clean bit sets here?

When we clean the set (e.g., the checkpoint is equal to size - 1) but the checkpoint can not advance to size. In this case, when size is marked as completed, checkpoint will still be equal to size - 1 but we have already cleaned the set.

bleskes · 2017-10-30T22:02:02Z

core/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java

-            FixedBitSet current = processedSeqNo.getFirst();
+            long bitArrayKey = getBitArrayKey(checkpoint);
+            FixedBitSet current = processedSeqNo.get(bitArrayKey);
+            if (current == null) {


since this is a FixedBitSet I wonder why renamed everything to use bitArray? I don't mind. Just curious.

Because I did not like the inconsistency that in some places it's referred to as a "bit array", and in some it's referred to as a "bit set". I wanted to make them all "bit set" but the problem is the setting index.seq_no.checkpoint.bit_arrays_size (currently never released to the world) refers to "bit array". If, and only if, you would be okay with changing the name of this setting to index.seq_no.checkpoint.bit_sets_size (in 6.0) then I would be okay with normalizing everything to "bit set".

I'm fine with renaming the setting. I think I added it just be able to control things from tests and as an extra escape hatch if the default size proved wrong. We might want to just remove it. We can solve the test part differently and I'm less concerned now about the default size (I'm also thinking that 1024 is maybe too small)..

I opened #27191.

bleskes · 2017-10-30T22:04:36Z

core/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java

+                 * current bit set, we can clean it.
+                 */
+                if (checkpoint == (1 + bitArrayKey) * bitArraysSize - 1) {
+                    assert current != null;


can we maybe cache (1 + bitArrayKey) * bitArraysSize - 1 to something like lastSeqNoInArray ? I think it will be easier to read.

I pushed 91fbab3.

bleskes · 2017-10-30T22:05:46Z

core/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java

+         * Previously this would allocate the entire chain of bit sets to the one for the sequence number being marked; for very large
+         * sequence numbers this could lead to excessive memory usage resulting in out of memory errors.
+         */
+        tracker.markSeqNoAsCompleted(randomNonNegativeLong());


lol. can we assert we allocated just one array?

I pushed b2cbfc5.

bleskes · 2017-10-30T22:08:15Z

core/src/test/java/org/elasticsearch/index/seqno/LocalCheckpointTrackerTests.java

        assertThat(tracker.getCheckpoint(), equalTo((long) localCheckpoint));
        assertThat(tracker.getMaxSeqNo(), equalTo((long) maxSeqNo));
-        assertThat(tracker.processedSeqNo, empty());
+        assertThat(tracker.processedSeqNo, new BaseMatcher<LongObjectHashMap<FixedBitSet>>() {


I despise assertTrue. Note that this gives a nicer error message if the assertion fails:

java.lang.AssertionError: Expected: empty but: was <[512=>org.apache.lucene.util.FixedBitSet@98761236]> Expected :empty Actual :<[512=>org.apache.lucene.util.FixedBitSet@98761236]>

as opposed to

java.lang.AssertionError at __randomizedtesting.SeedInfo.seed([F9E3DE31F5D979AE:C4C54170CE328934]:0) at org.junit.Assert.fail(Assert.java:86) at org.junit.Assert.assertTrue(Assert.java:41) at org.junit.Assert.assertTrue(Assert.java:52) at org.elasticsearch.index.seqno.LocalCheckpointTrackerTests.testResetCheckpoint(LocalCheckpointTrackerTests.java:276) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498)

jasontedor · 2017-10-30T22:27:44Z

Thanks @bleskes, I've responded to your feedback.

martijnvg

LGTM. Left one suggestion.

martijnvg · 2017-10-31T06:57:49Z

core/src/main/java/org/elasticsearch/index/seqno/LocalCheckpointTracker.java

+    }
+
+    private FixedBitSet getBitArrayForSeqNo(final long seqNo) {
+        assert Thread.holdsLock(this);


maybe use indexOf(...) and then indexInsert(...) and indexGet(...) respectively to avoid determining what the slot is for a key several times?

final int slot = processedSeqNo.indexOf(bitArrayKey); if (processedSeqNo.indexExists(slot) == false) { processedSeqNo.indexInsert(slot, bitArrayKey, new FixedBitSet(bitArraysSize)); } return processedSeqNo.indexGet(slot);

Thanks @martijnvg, I pushed cabda87.

And 68c4eab. 😐

Actually the indexGet at the end is not correct because the returned index is negative if the slot is not actually used. Yet, ~slot also does not work because the table could have been resized after the insert. I pushed 97bb3ca.

ah, I didn't realize that. Maybe we should do this to make optimal use of the slot?

final int slot = processedSeqNo.indexOf(bitArrayKey); if (processedSeqNo.indexExists(slot) == false) { FixedBitSet bitSet = new FixedBitSet(bitArraysSize)); processedSeqNo.indexInsert(slot, bitArrayKey, bitSet); return bitSet; } else { return processedSeqNo.indexGet(slot); }

When you see a chance to avoid == false, you seize it. 😉

I pushed f4f0dae.

* master: Remove checkpoint tracker bit sets setting Fix stable BWC branch detection logic Fix logic detecting unreleased versions Enhances exists queries to reduce need for `_field_names` (elastic#26930) Added new terms_set query Set request body to required to reflect the code base (elastic#27188) Update Docker docs for 6.0.0-rc2 (elastic#27166) Add version 6.0.0 Docs: restore now fails if it encounters incompatible settings (elastic#26933) Convert index blocks to cluster block exceptions (elastic#27050) [DOCS] Link remote info API in Cross Cluster Search docs page Fix Laplace scorer to multiply by alpha (and not add) (elastic#27125) [DOCS] Clarify migrate guide and search request validation Raise IllegalArgumentException if query validation failed (elastic#26811) prevent duplicate fields when mixing parent and root nested includes (elastic#27072) TopHitsAggregator must propagate calls to `setScorer`. (elastic#27138)

This local checkpoint tracker uses collections of bit sets to track which sequence numbers are complete, eventually removing these bit sets when the local checkpoint advances. However, these bit sets were eagerly allocated so that if a sequence number far ahead of the checkpoint was marked as completed, all bit sets between the "last" bit set and the bit set needed to track the marked sequence number were allocated. If this sequence number was too far ahead, the memory requirements could be excessive. This commit opts for a different strategy for holding on to these bit sets and enables them to be lazily allocated. Relates #27179

jasontedor changed the title ~~Lazy seq no bit sets~~ Lazy initialize checkpoint tracker bit sets Oct 30, 2017

jasontedor force-pushed the lazy-seq-no-bit-sets branch from 9581e2d to 21299b8 Compare October 30, 2017 20:55

jasontedor requested a review from bleskes October 30, 2017 20:55

jasontedor added :Sequence IDs v6.1.0 v7.0.0 >enhancement labels Oct 30, 2017

bleskes approved these changes Oct 30, 2017

View reviewed changes

jasontedor added 2 commits October 30, 2017 18:23

Method

91fbab3

Add assertion

b2cbfc5

martijnvg approved these changes Oct 31, 2017

View reviewed changes

jasontedor added 3 commits October 31, 2017 09:20

Use index

cabda87

Use index more

68c4eab

Revert

97bb3ca

jasontedor mentioned this pull request Oct 31, 2017

Remove checkpoint tracker bit sets setting #27191

Merged

bleskes mentioned this pull request Oct 31, 2017

Add Sequence Numbers to write operations #10708

Closed

64 tasks

jasontedor added 2 commits October 31, 2017 11:20

Oh yeah

f4f0dae

jasontedor merged commit 59657ad into elastic:master Nov 2, 2017

jasontedor deleted the lazy-seq-no-bit-sets branch November 2, 2017 01:27

clintongormley added :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Sequence IDs labels Feb 14, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

jasontedor commented Oct 30, 2017

Uh oh!

bleskes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor commented Oct 30, 2017

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijnvg Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

martijnvg Oct 31, 2017 •

edited

Loading