-
Notifications
You must be signed in to change notification settings - Fork 210
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
Implement getPartitioner() in HttpManagementProxy #1394
Implement getPartitioner() in HttpManagementProxy #1394
Conversation
@adejanovski this looks like it is failing to even build, is it draft? |
Sorry, it's a checkstyle issue that I missed. |
25687f0
to
efabcef
Compare
efabcef
to
fea52ad
Compare
fea52ad
to
f50c6c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions on this one below.
@@ -367,7 +367,7 @@ public Map<String, String> getEndpointToHostId() { | |||
} | |||
|
|||
@Override | |||
public String getPartitioner() { | |||
public String getPartitioner() throws ReaperException { | |||
Preconditions.checkNotNull(ssProxy, "Looks like the proxy is not connected"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Should this throw an exception now if the proxy is not connected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since the StorageServiceMbean.getPartitionerName()
signature doesn't throw anything, we can't properly get the errors.
I'll wrap this in a try/catch block for RuntimeExceptions and wrap it in a ReaperException.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll approve now and you can merge once you've made this change and all checks are passing.
...erver/src/test/java/io/cassandrareaper/management/http/HttpCassandraManagementProxyTest.java
Show resolved
Hide resolved
...erver/src/test/java/io/cassandrareaper/management/http/HttpCassandraManagementProxyTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving subject to the change being made to wrap potential runtime exceptions in a try/catch block
11676e4
into
integration/http-managementproxy
fixes #1381