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

Bugfix: added support for datacenter and rack names containing underscores #1129

Merged
merged 2 commits into from Oct 11, 2021
Merged

Bugfix: added support for datacenter and rack names containing underscores #1129

merged 2 commits into from Oct 11, 2021

Conversation

weisdd
Copy link
Contributor

@weisdd weisdd commented Oct 11, 2021

Issue

cassandra-reaper fails to properly parse datacenter and rack names that contain underscores.
Linked issue: #647

Suggested fix

I tracked it down to be a simple regexp issue - underscores were not included in:

  • ENDPOINT_DC_21_PATTERN
  • ENDPOINT_DC_22_PATTERN
  • ENDPOINT_RACK_21_PATTERN
  • ENDPOINT_RACK_22_PATTERN

After the change was applied for DC patterns, I can finally see them in UI (the ones that @a-nldisr mentioned: #647 (comment)):

image

Patterns for racks were adjusted in the same way.

P.S. I'm not a Java-developer by any means (I only code in Go/Python), so can't write unit-tests if it's something that you expect for this bugfix.

Signed-off-by: Igor Beliakov demtis.register@gmail.com

Signed-off-by: Igor Beliakov <demtis.register@gmail.com>
@weisdd weisdd changed the title Bugfix: added support for DCs with underscore Bugfix: added support for datacenter names containing underscores Oct 11, 2021
@adejanovski
Copy link
Contributor

Thanks for pushing this PR @weisdd !
I suppose it would be nice to do the same for racks indeed, we should honor the same constraints as naming of racks is free.
Don't worry about tests, I can add some afterwards and merge the PR in the meantime.
Let me know if you can make the change for racks and then if CI passes I can merge the PR.

@codecov
Copy link

codecov bot commented Oct 11, 2021

Codecov Report

Merging #1129 (5efdbe8) into master (b9c7129) will increase coverage by 0.22%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   73.90%   74.12%   +0.22%     
==========================================
  Files         109      109              
  Lines        8987     8987              
  Branches      931      931              
==========================================
+ Hits         6642     6662      +20     
+ Misses       1817     1794      -23     
- Partials      528      531       +3     
Impacted Files Coverage Δ
...io/cassandrareaper/resources/view/NodesStatus.java 91.72% <100.00%> (ø)
...a/io/cassandrareaper/storage/CassandraStorage.java 83.33% <0.00%> (-0.09%) ⬇️
.../java/io/cassandrareaper/service/RepairRunner.java 83.97% <0.00%> (+0.25%) ⬆️
...main/java/io/cassandrareaper/jmx/JmxProxyImpl.java 54.39% <0.00%> (+1.42%) ⬆️
...main/java/io/cassandrareaper/jmx/MetricsProxy.java 96.00% <0.00%> (+2.00%) ⬆️
...java/io/cassandrareaper/service/SegmentRunner.java 76.52% <0.00%> (+2.25%) ⬆️
...ain/java/io/cassandrareaper/jmx/SnapshotProxy.java 70.88% <0.00%> (+2.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9c7129...5efdbe8. Read the comment docs.

@weisdd
Copy link
Contributor Author

weisdd commented Oct 11, 2021

@adejanovski Sure, I'll add the change for racks soon.

Signed-off-by: Igor Beliakov <demtis.register@gmail.com>
@weisdd weisdd changed the title Bugfix: added support for datacenter names containing underscores Bugfix: added support for datacenter and rack names containing underscores Oct 11, 2021
@weisdd
Copy link
Contributor Author

weisdd commented Oct 11, 2021

@adejanovski I've just pushed the change and updated PR description. Please, approve the workflow execution :)

Copy link
Contributor

@adejanovski adejanovski 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 @weisdd

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.

None yet

2 participants