Repo analysis of uncontended register behaviour #101185

DaveCTurner · 2023-10-21T06:49:10Z

Today repository analysis verifies that a register behaves correctly
under contention, retrying until successful, but it turns out that some
repository implementations cannot even perform uncontended register
writes correctly which may cause endless retries in the contended case.
This commit adds another repository analyser which verifies that
uncontended register writes work correctly on the first attempt.

DaveCTurner · 2023-10-21T06:54:20Z

WIP because:

More robust timeout for repo analysis #101184 needs to go in first
Some of the renamings deserve to be split out into separate PRs
- Rename RegisterAnalyzeAction to ContendedR... #101192
No RepositoryAnalysisFailureIT test for this yet

Relates elastic#101185

Today repository analysis verifies that a register behaves correctly under contention, retrying until successful, but it turns out that some repository implementations cannot even perform uncontended register writes correctly which may cause endless retries in the contended case. This commit adds another repository analyser which verifies that uncontended register writes work correctly on the first attempt.

ywangd

LGTM

ywangd · 2023-10-23T01:34:02Z

test/fixtures/s3-fixture/src/main/java/fixture/s3/S3HttpHandler.java

+                for (final var multipartUpload : uploads.values()) {
+                    if (multipartUpload.getPath().startsWith(prefix)) {
+                        multipartUpload.appendXml(uploadsList);
+                    }


Interesting. This was essentially a bug in the fixture.

Yes, or at least an unimplemented feature :) This fixture takes quite a lot of shortcuts and only really emulates the bits of S3's API that matter to us.

ywangd · 2023-10-23T01:38:30Z

server/src/main/java/org/elasticsearch/TransportVersions.java

@@ -144,7 +144,7 @@ static TransportVersion def(int id) {
    public static final TransportVersion PIPELINES_IN_BULK_RESPONSE_ADDED = def(8_519_00_0);
    public static final TransportVersion PLUGIN_DESCRIPTOR_STRING_VERSION = def(8_520_00_0);
    public static final TransportVersion TOO_MANY_SCROLL_CONTEXTS_EXCEPTION_ADDED = def(8_521_00_0);
-
+    public static final TransportVersion UNCONTENDED_REGISTER_ANALYSIS_ADDED = def(8_522_00_0);


I think it's fine to use transportVersion for now. But strictly speaking, this feels more belong to the in-development Feature interface.

IMO adding a new remote action is a change to the transport protocol, although I do see that we could reasonably avoid calling the new action based on whether the cluster supports the feature or not. I don't expect we will be able to migrate assertions like these over to features tho (but maybe that is the eventual plan?):

https://github.com/elastic/elasticsearch/pull/101185/files#diff-73e6714a684296ea1da4ecb49e8b2e37485b08e60b44657ff7be05c7472b5181R154

ywangd · 2023-10-23T01:55:49Z

.../src/main/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalyzeAction.java

+            private final String registerName;
+            private final List<DiscoveryNode> nodes;
+            private final AtomicBoolean otherAnalysisComplete;
+            private int currentValue; // actions run in strict sequence so no need for synchronization


Sending request and handling response can be performed by different transport worker threads. So I think they can potentially see different values even when the action is in strict order?

I think this isn't the case, but it's a good observation. If that were the case then we potentially would need synchronization here indeed. Fortunately for remote requests the request and response go over the same TCP channel which means they use the same transport worker thread (docs) and therefore the request handling happens-before the response handling in program order on that thread so it's ok. Local requests bypass the transport worker threads of course, but they all happen within the same JVM so we have a proper happens-before relationship there too.

Thanks. You are right. In previous convesations, I heard that we could in theory send request via one channel but receive the response via another since transport does not have the ordered processing constraint of HTTP 1.1. It is a theoretical possbility, not what we have today. Sorry that I mis-remembered.

Technically speaking I think there may be no happens-before relationship between sending a request A and receiving a different request B which was caused by the remote node's handling of request A, because those things will use different TCP channels for sure and therefore may land on different transport threads. We do use nested requests in various places, e.g. recovery. I'm not sure if this is something that can happen in practice, but definitely something to watch out for.

ywangd · 2023-10-23T02:13:37Z

.../java/org/elasticsearch/repositories/blobstore/testkit/UncontendedRegisterAnalyzeAction.java

+                            // Registers are not supported on all repository types, and that's ok.
+                            listener.onResponse(null);


For my understanding: I don't think we indicate in the response that this operation is unsupported? Are we not interested in it? I am aware that the existing "Contented" version does the same. So it is likely ok.

That's correct, specifically we do not support register operations for the (somewhat-unloved) HDFS repository implementation, and we have no plan to address this in future so we just skip all these checks for HDFS repositories.

ywangd · 2023-10-23T03:38:07Z

...rTest/java/org/elasticsearch/repositories/blobstore/testkit/RepositoryAnalysisSuccessIT.java

+            } else if (key.startsWith(RepositoryAnalyzeAction.UNCONTENDED_REGISTER_NAME_PREFIX) || randomBoolean()) {
                listener.onResponse(OptionalBytesReference.of(registers.computeIfAbsent(key, ignored -> new BytesRegister()).get()));
            } else {
                final var bogus = randomFrom(BytesArray.EMPTY, new BytesArray(new byte[] { randomByte() }));


This logic here is a bit to follow. IIUC, we don't want to return anything wrong for uncontended analysis. But the code here seems to suggest that we could be returning bogus result for it. But not really because the call to compareAndExchangeRegister always return a new BytesRegister() which always return 0 regardless of the bogus value. I think it would be better if we could make this more explicit for the uncontended operations. Maybe have it as the top level switch, e.g.:

if (key.startsWith(RepositoryAnalyzeAction.UNCONTENDED_REGISTER_NAME_PREFIX)) { ... } else { // everything else, essentially the existing code }

?

++ yes this is all a little unsatisfactory indeed. This area is going to need a little rework once #101184 is merged, I'll try and bring the checks on the key prefix to the top level in these methods.

Ok now merged and cleaned up, see 2dd4250.

Relates #101185

elasticsearchmachine · 2023-10-23T09:03:51Z

Hi @DaveCTurner, I've created a changelog YAML for you.

elasticsearchmachine · 2023-10-23T09:03:51Z

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

fcofdez

LGTM

Verification of uncontended operations on linearizable registers was introduced in elastic#101185 so does not apply in versions before 8.12. Fixes up the backport of elastic#102050.

Verification of uncontended operations on linearizable registers was introduced in #101185 so does not apply in versions before 8.12. Fixes up the backport of #102050.

Verification of uncontended operations on linearizable registers was introduced in elastic#101185 so does not apply in versions before 8.12. Fixes up the backport of elastic#102050.

Verification of uncontended operations on linearizable registers was introduced in #101185 so does not apply in versions before 8.12. Fixes up the backport of #102050.

DaveCTurner added >enhancement WIP :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Supportability Improve our (devs, SREs, support eng, users) ability to troubleshoot/self-service product better. v8.12.0 labels Oct 21, 2023

DaveCTurner mentioned this pull request Oct 21, 2023

Repository analysis timeout should apply to register operations #101182

Closed

DaveCTurner force-pushed the 2023/10/21/uncontended-register-analysis branch 2 times, most recently from 854c212 to 9d3cdb9 Compare October 21, 2023 11:22

DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request Oct 21, 2023

Rename RegisterAnalyzeAction to ContendedR...

1018b00

Relates elastic#101185

DaveCTurner mentioned this pull request Oct 21, 2023

Rename RegisterAnalyzeAction to ContendedR... #101192

Merged

DaveCTurner force-pushed the 2023/10/21/uncontended-register-analysis branch from 9d3cdb9 to 17566ba Compare October 21, 2023 16:02

DaveCTurner requested a review from ywangd October 22, 2023 20:51

DaveCTurner marked this pull request as ready for review October 22, 2023 20:51

ywangd approved these changes Oct 23, 2023

View reviewed changes

DaveCTurner added 3 commits October 23, 2023 08:44

Merge branch 'main' into 2023/10/21/uncontended-register-analysis

9d8fc36

Clean up register disruption logic

c822005

Static imports

40531bb

elasticsearchmachine pushed a commit that referenced this pull request Oct 23, 2023

Rename RegisterAnalyzeAction to ContendedR... (#101192)

a1c1883

Relates #101185

Merge branch 'main' into 2023/10/21/uncontended-register-analysis

2dd4250

DaveCTurner requested a review from fcofdez October 23, 2023 09:03

DaveCTurner removed the WIP label Oct 23, 2023

elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Oct 23, 2023

Update docs/changelog/101185.yaml

699e0cb

DaveCTurner added 3 commits October 23, 2023 10:41

Assert exception

ca2248d

Test cax reads of uncontended register

05de87b

Reduce diff

d4ac9b1

fcofdez approved these changes Oct 23, 2023

View reviewed changes

DaveCTurner merged commit 4bbf760 into elastic:main Oct 23, 2023

DaveCTurner deleted the 2023/10/21/uncontended-register-analysis branch October 23, 2023 10:46

DaveCTurner mentioned this pull request Nov 15, 2023

Remove docs on uncontended register analysis #102201

Merged

DaveCTurner restored the 2023/10/21/uncontended-register-analysis branch June 17, 2024 06:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repo analysis of uncontended register behaviour #101185

Repo analysis of uncontended register behaviour #101185

DaveCTurner commented Oct 21, 2023 •

edited

Loading

DaveCTurner commented Oct 21, 2023 •

edited

Loading

ywangd left a comment

ywangd Oct 23, 2023

DaveCTurner Oct 23, 2023

ywangd Oct 23, 2023

DaveCTurner Oct 23, 2023

ywangd Oct 23, 2023

DaveCTurner Oct 23, 2023

ywangd Oct 23, 2023

DaveCTurner Oct 23, 2023

ywangd Oct 23, 2023

DaveCTurner Oct 23, 2023

ywangd Oct 23, 2023

DaveCTurner Oct 23, 2023

DaveCTurner Oct 23, 2023

elasticsearchmachine commented Oct 23, 2023

elasticsearchmachine commented Oct 23, 2023

fcofdez left a comment

		// Registers are not supported on all repository types, and that's ok.
		listener.onResponse(null);

Repo analysis of uncontended register behaviour #101185

Repo analysis of uncontended register behaviour #101185

Conversation

DaveCTurner commented Oct 21, 2023 • edited Loading

DaveCTurner commented Oct 21, 2023 • edited Loading

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 23, 2023

elasticsearchmachine commented Oct 23, 2023

fcofdez left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 21, 2023 •

edited

Loading

DaveCTurner commented Oct 21, 2023 •

edited

Loading