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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.io.IOException;
import java.util.function.Supplier;

import com.google.common.annotations.VisibleForTesting;
import com.palantir.logsafe.Preconditions;

import okhttp3.OkHttpClient;
Expand Down Expand Up @@ -46,13 +47,32 @@ public HostLocation get() {
Preconditions.checkState(response.isSuccessful(),
"Getting AWS host metadata was not successful");

String responseString = response.body().string();
String datacenter = responseString.substring(0, responseString.length() - 2);
String rack = responseString.substring(responseString.length() - 1);

return HostLocation.of(datacenter, rack);
return parseHostLocation(response.body().string());
} catch (IOException e) {
throw new RuntimeException(e);
}
}

@VisibleForTesting
static HostLocation parseHostLocation(String az) {
// Code is copied from Cassandra's Ec2Snitch. The result of this parsing must match Cassandra's as closely as
// 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


// Split "us-east-1a" or "asia-1a" into "us-east"/"1a" and "asia"/"1a".
String[] splits = az.split("-");
ec2zone = splits[splits.length - 1];

// hack for CASSANDRA-4026
ec2region = az.substring(0, az.length() - 1);
if (ec2region.endsWith("1")) {
ec2region = az.substring(0, az.length() - 3);
}

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

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,10 @@ public void shouldReturnEmptyLocationFromEc2Exception() {
assertThat(hostLocationSupplier.get()).isNotPresent();
}

@Test
public void shouldReturnHostLocationInCassandraStyle() {
HostLocation awsLocation = HostLocation.of("us-east", "1a");
assertThat(Ec2HostLocationSupplier.parseHostLocation("us-east-1a")).isEqualTo(awsLocation);
}

}
18 changes: 18 additions & 0 deletions changelog/@unreleased/pr-4283.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
type: fix
fix:
description: |-
Fix Ec2HostLocationSupplier using a different format to Cassandra

**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
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).

links:
- https://github.com/palantir/atlasdb/pull/4283