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

Add PLACEMENT_AWARE partition group support. #221

Merged
merged 10 commits into from
Jan 22, 2021

Conversation

enozcan
Copy link
Contributor

@enozcan enozcan commented Dec 31, 2020

Adds placement aware partition group strategy support to form partition groups based on AWS placement groups. The strategy provides availability within a single zone as high as possible by spreading partitions and their replicas across different racks.

@enozcan enozcan marked this pull request as ready for review January 11, 2021 12:53
Copy link

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added some comments. The code looks fine, most comments are actually to README.

Additionally, I think we should merge PR after hazelcast/hazelcast#18048

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/main/java/com/hazelcast/aws/AwsDiscoveryStrategy.java Outdated Show resolved Hide resolved
@enozcan
Copy link
Contributor Author

enozcan commented Jan 13, 2021

@leszko PTAL

@enozcan
Copy link
Contributor Author

enozcan commented Jan 13, 2021

Per hazelcast/hazelcast#18048 (comment), replaced @ notation in PPG group name by -

Copy link

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added 3 comments to README. Other than that, LGTM 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@enozcan
Copy link
Contributor Author

enozcan commented Jan 16, 2021

I noticed that with the current setup it keeps retrying even after 404 response is received. This causes instances to start after ~15 seconds of delay with 5 retry count - even an instance does not belong to a placement group. As the current rest client expects only HTTP_OK and assumes failed otherwise, the rest client needs some refactoring to overcome this.

I added the required changes to this PR via 88849d7 but it might be difficult to review them all together here. I can separate this into two different PRs: 1) Rest client refactoring and 2) Placement Aware support.

@leszko Please let me know your opinion and I'm sorry for the inconvenience.

What 88849d7 does is:

  • Wrapping response messages along with the response code into RestClient.Response object.
  • Making rest calls to expect certain response codes (via RestClient#expectResponseCodes(Integer... codes)) and if responses do not match, then throw exception. By default, 200 is the only expected code - the same as the current behavior.
  • Allowing rest clients to read error messages also (RestClient#read). With the current versions clients can only read response bodies returned with 200.
  • When resolving placement groups, it retries until 200 or 404 is returned. If it failed to get none of them after retries, it logs a warning.

Copy link

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added one minor comment. Other than that, looks good. Thanks @enozcan for the changes.

src/main/java/com/hazelcast/aws/AwsMetadataApi.java Outdated Show resolved Hide resolved
Co-authored-by: Rafał Leszko <rafal@hazelcast.com>
Copy link
Contributor

@alparslanavci alparslanavci left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks for the clean code.

@enozcan enozcan merged commit 94420e2 into hazelcast:master Jan 22, 2021
@mtyazici mtyazici added this to the 3.3.1 milestone Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants