Skip to content

Add metric with neighbours for each token range - #9

Open
serban21 wants to merge 1 commit into
masterfrom
neighbours
Open

Add metric with neighbours for each token range#9
serban21 wants to merge 1 commit into
masterfrom
neighbours

Conversation

@serban21

@serban21 serban21 commented Feb 5, 2020

Copy link
Copy Markdown
Owner

No description provided.

final Stream<NumericMetric> ownershipMetricStream = metadataFactory.keyspaces().stream()
.filter(keyspace -> !excludedKeyspaces.contains(keyspace))
.flatMap(keyspace -> {
try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In order to increase readability for this method, what do you think about extracting this block into a dedicated method? I imagine it could return a Stream<...>

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I agree, and I'll remember this. But if you look further down (see effectiveOwnership(keyspace)) in the code you'll see that's not how the code is written, so for now I'd keep the same code style.

// token range example:
// TokenRange(start_token:5585272669612250202, end_token:5664918566912044362, endpoints:[172.16.28.48, 172.16.28.166], rpc_endpoints:[172.16.28.48, 172.16.28.166], endpoint_details:[EndpointDetails(host:172.16.28.48, datacenter:eu-west_edge-irl1_profiles-bk, rack:1a), EndpointDetails(host:172.16.28.166, datacenter:eu-west_edge-irl1_profiles-bk, rack:1c)])
// see https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/service/TokenRange.java
String s = tokenRange.substring(tokenRange.indexOf(", endpoints:") + 13);

@costimuraru costimuraru Feb 6, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are the token ranges always in valid format? If not, should we have a regex to validate them before going ahead with these splits/substrings?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

The format is fixed, and it's very unlikely to change in the future. It seems to have been added just to preserve some backward compatibility, and it's unlikely to ever change. It's used only by a nodetool option, nodedetool describering. Anyway, I'll change it to a regex match and group, it's probably safer anyway.

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.

3 participants