Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

Fix Ec2HostLocationSupplier using a different format to Cassandra #4283

Merged
merged 8 commits into from
Oct 7, 2019

Conversation

Jolyon-S
Copy link
Contributor

@Jolyon-S Jolyon-S commented Oct 7, 2019

Goals (and why):
Modify Ec2HostLocationSupplier to emulate Cassandra's Ec2Snitch logic (as it is later comparing against parsed info from that class).

Implementation Description (bullets):
The response string from the AWS endpoint is now parsed using code copied from Cassandra's implementation (or as close as possible)

Testing (What was existing testing like? What have you done to improve it?):
Added a test to make sure that the parsing gives the expected result

Priority (whenever / two weeks / yesterday):
Today

@changelog-app
Copy link

changelog-app bot commented Oct 7, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Modify Ec2HostLocationSupplier to emulate Cassandra's Ec2Snitch logic. Previously, Ec2HostLocationSupplier may have produced results inconsistent with Cassandra's Ec2Snitch, meaning that our feature to send more traffic to local nodes wouldn't work properly

Check the box to generate changelog(s)

  • Generate changelog entry


return HostLocation.of(ec2region, ec2zone);


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We don't normally leave whitespace after the return statement

// possible, as the strings are later matched exactly.

String ec2region;
String ec2zone;
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-actionable: This is fine and I guess copied from original, though we normally prefer to declare the variables as they occur

Added a test to make sure that the parsing gives the expected result

**Priority (whenever / two weeks / yesterday)**:
Today
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't want to include all of this in the changelog. Most of the time, something along the lines of the initial message (i.e. "Ec2HostLocationSupplier now users a format consistent with Cassandra's Ec2Snitch") is sufficient.

Agree this is a fix - though for fixes it's usually good to say what the broken behaviour was, and what impact that might have had on users on the version without the fix (i.e. "Ec2HostLocationSupplier may have produced results inconsistent with Cassandra's Ec2Snitch, meaning that our feature to send more traffic to local nodes wouldn't work properly" or something like that).

Copy link
Contributor

@jeremyk-91 jeremyk-91 left a comment

Choose a reason for hiding this comment

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

Looks good!

@jeremyk-91 jeremyk-91 merged commit a10e2a9 into develop Oct 7, 2019
@jeremyk-91 jeremyk-91 deleted the fixEC2HostLocationSupplier branch October 7, 2019 15:33
@svc-autorelease
Copy link
Collaborator

Released 0.158.4

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants