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

node_probe: toppartitions: Fix wrong class in getMethod #256

Merged

Conversation

StarostaGit
Copy link

We check for the existence of the new toppartitions JMX API
using Class.getMethod, but use wrong class for List
in parameter list. This change fixes that.

We check for the existence of the new toppartitions JMX API
using Class.getMethod, but use wrong class for List<String>
in parameter list. This change fixes that.
@StarostaGit StarostaGit requested review from amnonh and avikivity June 3, 2021 10:40
@amnonh
Copy link
Contributor

amnonh commented Jun 3, 2021

typically, nodeprobe is taken from Cassandra as-is, why is it different in this case?

@StarostaGit
Copy link
Author

StarostaGit commented Jun 3, 2021

As is? Isn't NodeProbe.javaour own version we use to communicate with JMX? I changed this code throughout multiple PRs. And this is an ammendment to one of those

@amnonh
Copy link
Contributor

amnonh commented Jun 3, 2021

No, nodeprobe connect to scylla-jmx.
for a very long time, nodeprob and nodetool in general were Cassandra compatible and while this rule no longer holds, there should be a reason why it's not.
Is this something that got broken because of other change?

@StarostaGit
Copy link
Author

No, nodeprobe connect to scylla-jmx.
for a very long time, nodeprob and nodetool in general were Cassandra compatible and while this rule no longer holds, there should be a reason why it's not.
Is this something that got broken because of other change?

Yes, it's the method introduced to handle the generic toppartitions calls, #250 introduced the try...catch for legacy calls and I forgot to check that when both are present the newer one gets called. And the bug was because of the wrong Class object passed to getMethod

@amnonh
Copy link
Contributor

amnonh commented Jun 6, 2021

LGTM

@avikivity avikivity merged commit 1a00241 into scylladb:master Jun 6, 2021
@bhalevy
Copy link
Member

bhalevy commented Jun 16, 2021

@avikivity can you please update the tools/java submodule in scylla to include this fix?

@bhalevy
Copy link
Member

bhalevy commented Jun 16, 2021

Also @scylladb/scylla-maint (doesn't have to be Avi.... :))

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.

4 participants