Update sparse file tracker `complete` pointer on progress by tlrx · Pull Request #109247 · elastic/elasticsearch

tlrx · 2024-05-31T14:20:53Z

This change updates the complete pointer when a Gap makes progress.

henningandersen

Looks great. Left a few smaller comments, but I think moving forward makes sense.

henningandersen · 2024-06-03T08:36:13Z

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

-    private volatile List<PositionAndListener> listeners;
-    protected volatile long progress;
+    /**
+     * A consumer that accepts progress made by this {@link ProgressListenableActionFuture}


Can we add this to the constructor javadoc too. And also mention that this is called prior to notifying listeners (since we'll rely on that), though with out of order possibilities for concurrent notifications.

I pushed b15d351

henningandersen · 2024-06-03T08:37:57Z

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

+            consumer.accept(progress);
+        } catch (Exception e) {
+            assert false : e;
+            throw e;


I'd probably prefer to log a warning instead rather than rethrow. There seems to be no reason to throw out the exception if the consumer were to fail (I am sure it will not for now).

Sure, I pushed b2249a6. I don't expect this to throw though.

...ck/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/common/SparseFileTracker.java

henningandersen · 2024-06-03T08:50:19Z

...ck/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/common/SparseFileTracker.java

+
+    private void updateCompletePointerHoldingLock(long value) {
+        assert Thread.holdsLock(ranges);
+        assert complete <= value : complete + ">" + value;


When called from maybeUpdateCompletePointer, this assertion depends on ProgressListenableActionFuture not notifying of progress on the last byte. It feels slightly subtle to me, but works ofc. The check I suggested in maybeUpdateCompletePointer should fix it though.

elasticsearchmachine · 2024-06-05T11:36:53Z

Pinging @elastic/es-distributed (Team:Distributed)

tlrx · 2024-06-05T11:38:03Z

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

        synchronized (this) {
            assert completed == false;
            completed = true;
+            assert listeners == null || listeners.stream().allMatch(l -> progress < l.position() && l.position() <= end);


Relates https://github.com/elastic/elasticsearch-serverless/pull/1872#pullrequestreview-2033093453

Hmm the actual length of the data is actionResult() which should be <= end. When actionResult() < end, I think there should be no listener waiting at all when we reach here? Otherwise these listeners are waiting for a range that can never be fulfilled?

the actual length of the data is actionResult() which should be <= end.

I think that the cache does not allow to partially complete ranges, so it expects the range (gap) to be fully written or it considers it failed and any remaining listener will be failed too.

I pushed cdabee7 to ensure it is always completed with a value equals to end.

OK I see. The SparseFileTracker always completes ProgressListenableActionFuture with the range end. So regardless of the progress this range has actually made, the actionResult() should be equal to end when it completes.

ywangd

With these changes, I wonder whether we still need the shortcircuting for when progressValue == end introduced in #108095?

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

ywangd · 2024-06-06T08:06:26Z

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

        synchronized (this) {
            assert completed == false;
            completed = true;
+            assert listeners == null || listeners.stream().allMatch(l -> progress < l.position() && l.position() <= end);


Hmm the actual length of the data is actionResult() which should be <= end. When actionResult() < end, I think there should be no listener waiting at all when we reach here? Otherwise these listeners are waiting for a range that can never be fulfilled?

tlrx · 2024-06-06T09:16:44Z

Thanks for your feedback @ywangd!

I wonder whether we still need the shortcircuting for when progressValue == end introduced in #108095?

I see what you mean bu I still think it makes sense to not complete "early" the listeners due to progress being made if those listeners are waiting for the full gap to be completed. Feels more correct to me.

ywangd

Thanks for answering my question, Tanguy. I have a few more minor ones. This is looking great. I just need another pass for the tests before approval. Thank you!

ywangd · 2024-06-07T03:20:03Z

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

+    @Nullable
+    private final LongConsumer progressConsumer;
+
+    private List<PositionAndListener> listeners;


I think we still need volatile here or alternatively make toString synchronized?

Let's not bring back volatile only for toString(), I pushed af58294

ywangd · 2024-06-07T03:35:52Z

...ck/plugin/blob-cache/src/main/java/org/elasticsearch/blobcache/common/SparseFileTracker.java

+        if (rangeStart == complete) {
+            return this::updateCompletePointer;
+        } else {
+            return null;
+        }


Mostly making sure I understand how this works. We don't update complete unless every byte before it is fetched. We also adjust targetRange.start so that we can expect one of the ranges will resume fetching from complete. And that range is the only one that should update the complete field?

ywangd

LGTM

ywangd · 2024-06-11T09:13:29Z

...he/src/test/java/org/elasticsearch/blobcache/common/ProgressListenableActionFutureTests.java

+        var future = new ProgressListenableActionFuture(
+            start,
+            end,
+            p -> assertThat("LongConsumer should not consumed the same  value twice", consumed.add(p), equalTo(true))


Nit: extra space :)

Suggested change

p -> assertThat("LongConsumer should not consumed the same value twice", consumed.add(p), equalTo(true))

p -> assertThat("LongConsumer should not consumed the same value twice", consumed.add(p), equalTo(true))

Eagle eyes again :)

ywangd · 2024-06-11T09:18:03Z

...he/src/test/java/org/elasticsearch/blobcache/common/ProgressListenableActionFutureTests.java

+    public void testLongConsumerCalledOnProgressUpdate() {
+        // min length of 2 to have at least one progress update before reaching the end
+        long length = randomLongBetween(2L, ByteSizeUnit.TB.toBytes(1L));
+        long start = randomLongBetween(Long.MIN_VALUE, Long.MAX_VALUE - length);


The code does not prevent negative start values. But it seems rather odd to allow it. I wonder whether we should tighten it up in a separate PR to assert start >= 0?

ywangd · 2024-06-11T09:21:51Z

...he/src/test/java/org/elasticsearch/blobcache/common/ProgressListenableActionFutureTests.java

+        );
+
+        long position = start;
+        for (int i = 0; i < 25 && position < end - 1L; i++) {


Nit: Maybe a comment about why 25 is used could be helpful.

This is just a number of iterations, I used a local variable in 16052d4

ywangd · 2024-06-11T09:27:16Z

...he/src/test/java/org/elasticsearch/blobcache/common/ProgressListenableActionFutureTests.java

+            assertThat(consumed.contains(progress), equalTo(true));
+            assertThat(listener.isDone(), equalTo(true));
+            position = progress;
+        }


The consumer is not called for future.onProgress(end). Is there any value to assert that? Or do you think this is an implementation detail and not worth it?

I added a check that the consumer is not called when progress is updated to end in 16052d4

ywangd · 2024-06-11T09:32:10Z

...ugin/blob-cache/src/test/java/org/elasticsearch/blobcache/common/SparseFileTrackerTests.java

+                    if (awaitingListener != null && j < gap.end() - 1L) {
+                        assertThat(
+                            "Complete pointer should have been updated when a listener is waiting for the gap to be completed",
+                            tracker.getComplete(),
+                            equalTo(j + 1L)
+                        );
+                        latestUpdatedCompletePointer = tracker.getComplete();


I think this branch meant to assert awaitingKListener.isDone?

Sure, pushed in 16052d4

ywangd · 2024-06-11T09:36:54Z

...b-cache/src/main/java/org/elasticsearch/blobcache/common/ProgressListenableActionFuture.java

+    /**
+     * A consumer that accepts progress made by this {@link ProgressListenableActionFuture}. The consumer is called before listeners are
+     * notified of the updated progress value in {@link #onProgress(long)}. The consumer can be called with out-of-order progress values.
+     */


Can we call out that the consumer is not called when progress == end? I find it important and a bit counter-intuitive. So it's helpful to explicitly note it down.

Done in 16052d4

tlrx · 2024-06-12T06:58:48Z

Thanks Yang & Henning!

[Draft] Update sparse file tracker complete pointer on progress

fb220f6

tlrx added >non-issue :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v8.15.0 labels May 31, 2024

tlrx mentioned this pull request May 31, 2024

Do not execute listeners when progress is updated to the end of the range #108095

Merged

henningandersen self-requested a review June 3, 2024 08:33

henningandersen reviewed Jun 3, 2024

View reviewed changes

tlrx added 6 commits June 4, 2024 15:56

Merge branch 'main' into 2024/05/31/update-complete-marker-on-progress

4c5af39

doc

b15d351

logger

b2249a6

tests

5bdc86c

assert

c12407d

Merge branch 'main' into 2024/05/31/update-complete-marker-on-progress

a4835cc

tlrx changed the title ~~[Draft] Update sparse file tracker complete pointer on progress~~ Update sparse file tracker complete pointer on progress Jun 5, 2024

tlrx marked this pull request as ready for review June 5, 2024 11:36

tlrx requested review from henningandersen and ywangd June 5, 2024 11:36

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Jun 5, 2024

tlrx commented Jun 5, 2024

View reviewed changes

ywangd reviewed Jun 6, 2024

View reviewed changes

fix onResponse

cdabee7

Merge branch 'main' into 2024/05/31/update-complete-marker-on-progress

61f76ae

ywangd reviewed Jun 7, 2024

View reviewed changes

tlrx added 3 commits June 7, 2024 11:46

Merge branch 'main' into 2024/05/31/update-complete-marker-on-progress

2f3a7a1

sync toString

af58294

Merge branch 'main' into 2024/05/31/update-complete-marker-on-progress

ae06084

tlrx requested a review from ywangd June 7, 2024 15:11

ywangd approved these changes Jun 11, 2024

View reviewed changes

tlrx added 2 commits June 11, 2024 12:28

Merge branch 'main' into 2024/05/31/update-complete-marker-on-progress

d608d2c

feedback

16052d4

tlrx merged commit a7cd581 into elastic:main Jun 12, 2024

tlrx deleted the 2024/05/31/update-complete-marker-on-progress branch June 12, 2024 06:58

	p -> assertThat("LongConsumer should not consumed the same value twice", consumed.add(p), equalTo(true))
	p -> assertThat("LongConsumer should not consumed the same value twice", consumed.add(p), equalTo(true))

Conversation

tlrx commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henningandersen 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jun 5, 2024

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!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jun 6, 2024

Uh oh!

ywangd 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!

ywangd 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!

tlrx commented Jun 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tlrx commented May 31, 2024 •

edited

Loading