Skip to content

Conversation

@joegallo
Copy link
Contributor

@joegallo joegallo commented Mar 21, 2023

As requested in #92308 (comment) (which I'm finally getting back to, yikes!).

Instead of a compareAndSet followed by an accumulateAndGet, just use a single accumulateAndGet compareAndExchange for everything.

I'm 97.5% sure the logic of this remains correct, and I think it's simpler this way, but if somebody can point out a bug in the logic or disagrees about it being simpler, I'm happy to change or close this PR.

For a bit of the history on this, see https://github.com/elastic/elasticsearch/pull/23969 (code highlighting to avoid a github reference being added), as well as #37649 and #53262.

Instead of a compareAndSet followed by an accumulateAndGet, just use a
single accumulateAndGet for everything.
@joegallo joegallo added :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.8.0 labels Mar 21, 2023
@joegallo joegallo requested review from DaveCTurner and thecoop March 21, 2023 18:52
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all seems very complicated compared with the approach in RefCountingListener. Either this can be simplified, or RefCountingListener has a bug...

final var firstException = exceptionRef.compareAndExchange(null, e);
if (firstException != null && firstException != e) {
firstException.addSuppressed(e);
}

@joegallo
Copy link
Contributor Author

joegallo commented Mar 21, 2023

I'm good with that solution. addSuppressed is already synchronized, so I think your approach would work fine. edit: Done, 5209fe7.

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@thecoop thecoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible!

@joegallo joegallo merged commit 0747753 into elastic:main Mar 23, 2023
@joegallo joegallo deleted the failure-accumulate-and-get branch March 23, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants