Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added retry logic if client request returns an error code that is configured for retrying #4409

Merged

Conversation

criminosis
Copy link
Contributor

@criminosis criminosis commented Apr 22, 2024

Closes #4408

To wait between retries since the code being called into Elasticsearch's client appears to be the synchronous API I opted for just using Thread.sleep. If there's a better utility or if a different approach is desired happy change.

Additionally I marked the new configuration options with the LOCAL flag. I noticed retry_on_conflict went with MASKABLE but it didn't seem readily apparent what further obligations would be needed to fulfill the "global" portion of that contract, so just figured it'd be best to start with LOCAL and ask about it in the PR.


Thank you for contributing to JanusGraph!

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there an issue associated with this PR? Is it referenced in the commit message?
  • Does your PR body contain #xyz where xyz is the issue number you are trying to resolve?
  • Has your PR been rebased against the latest commit within the target branch (typically master)?
  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you written and/or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE.txt file, including the main LICENSE.txt file in the root of this repository?
  • If applicable, have you updated the NOTICE.txt file, including the main NOTICE.txt file found in the root of this repository?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

@janusgraph-bot janusgraph-bot added the cla: external Externally-managed CLA label Apr 23, 2024
@criminosis criminosis force-pushed the elasticsearch_backpressure_backoff branch from 2a30ce3 to 1bccf28 Compare April 24, 2024 03:51
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this! My two cents:

  1. Either local or maskable is fine. Maskable means the change will be (automatically) propagated to different instances, but every instance could override the value locally. Local is simpler and has less overhead (as no propagation is needed). I would prefer to use LOCAL here.
  2. It's okay to write our own exponential backoff code.

@li-boxuan li-boxuan added this to the Release v1.1.0 milestone Apr 24, 2024
@criminosis criminosis force-pushed the elasticsearch_backpressure_backoff branch from 5354c7d to 340b591 Compare April 24, 2024 05:15
Copy link
Member

@li-boxuan li-boxuan left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@li-boxuan li-boxuan requested a review from porunov April 24, 2024 06:56
Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

Thank you @criminosis ! I have just some nitpicks below. Otherwise looks good.

Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

@criminosis please, feel free to squash the commits. However, notice that ElasticSearch tests are now failing:

Failures: 
Error:    RestClientSetupTest.testConnectBasicHttpConfigurationAllOptions:257 expected: <[408, 429]> but was: <null>
Error:  Errors: 
Error:    RestClientSetupTest.testSSLDisableHostNameVerifierDefaultOff » InvalidUseOfMatchers 

…figured for retrying

Closes JanusGraph#4408

Added additional tests for coverage

Included example of comma separated values for retry-error-codes config parameter

Simplified retry loop logic

Moved retry configurations into constructor and removed setters

Signed-off-by: Allan Clements <criminosis@gmail.com>
@criminosis criminosis force-pushed the elasticsearch_backpressure_backoff branch from c3961fc to d75cfe6 Compare April 26, 2024 22:41
@criminosis
Copy link
Contributor Author

@porunov yeah, I just focused on compilation passing to present the new signatures in case you had feedback or further modification, etc.

Fixed it all up, squashed and re-signed off again. Should be ready for review again 👍

Copy link
Member

@porunov porunov left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your contribution @criminosis !

@criminosis
Copy link
Contributor Author

Thank you @porunov & @li-boxuan , glad to finally get to find something to contribute back to JanusGraph!

Out of curiosity I see it's been earmarked for the 1.1.0 milestone. Assuming the build completely passes and it gets merged will it go out into nightly build / release prior to the 1.1.0 release?

Looking forward to getting this my cluster to see if it helps the issue that originally prompted this.

@porunov
Copy link
Member

porunov commented Apr 26, 2024

Thank you @porunov & @li-boxuan , glad to finally get to find something to contribute back to JanusGraph!

Out of curiosity I see it's been earmarked for the 1.1.0 milestone. Assuming the build completely passes and it gets merged will it go out into nightly build / release prior to the 1.1.0 release?

Looking forward to getting this my cluster to see if it helps the issue that originally prompted this.

Yes. If everything passes the snapshot release is automatically created for each merged commit for any supported branches (master and v1.0 at the moment). You can read about snapshot releases here: https://docs.janusgraph.org/advanced-topics/commit-releases/

@porunov porunov merged commit 4bddfb4 into JanusGraph:master Apr 27, 2024
174 checks passed
@criminosis criminosis deleted the elasticsearch_backpressure_backoff branch April 27, 2024 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: external Externally-managed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Retry Logic For Elasticsearch Requests
4 participants