Nodetool over REST #121

tarzanek · 2019-11-15T15:35:31Z

DTest is done in
https://jenkins.scylladb.com/view/master/job/scylla-master/job/manual-and-scheduled-tests/job/byo_build_tests_dtest/653/

tarzanek · 2019-11-16T11:26:39Z

@elcallio @amnonh @haaawk hi guys, if you will have a moment, please review
diff to pull request #100 (original one) - added data holder jmxbeans to get cfstats working without jmx (thnx Calle), added all jars as dependencies to lib (thnx Piotr)

this is the first step, basically after this moving all the rest of RESTNodeProbe to REST is just a matter of moving the rest calls from scylla-jmx to the probe (I don't expect such troubles as I had with cfstats dtests)

also @bhalevy - I've only run dtest in byo , are there some better test suites to run for nodetool compliance? (so it behaves and reports same values like the old one?)

nodetool compactionstats nodetool describecluster nodetool tablestats nodetool tablehistograms nodetool status nodetool info nodetool cfstats

tarzanek · 2019-11-27T12:21:01Z

reruning dtest after adding all commands the were requested:
https://jenkins.scylladb.com/job/scylla-master/job/debug-jobs/job/byo_build_tests_dtest/704/

amnonh · 2019-11-27T14:39:09Z

I have a general concern, I think that copying the logic from scylla-jmx to the nodetool is the wrong way around it.

For the long term, I think that taking the objects from scylla-jmx and plug them inside node_probe is the better approach.

This way is more robust to changes both in the API and in nodetool.

tarzanek · 2019-11-27T16:14:46Z

@amnonh hmm, I agree, but that would mean I will need to refactor scylla-jmx even more and break it to smaller pieces again (same way how I extracted api client)
would that be feasible?

tarzanek · 2019-11-27T16:17:48Z

@amnonh also I will have to make weird changes to scylla-jmx, since lots of mxbeans have private access + lack simple constructors (and I will need to add compatibility constructors)
so the impact will be that the scylla-jmx mxbeans will be more open (and possible unsafe, but this can be a lesser concern than duplicating of logic/code)

also - how to deduplicate the same class names from different jars? (I'd have to do a special shading it seems, or you have a better idea?)
e.g. CompactionManager is in both projects in same namespace (org.apache.cassandra.db.compaction) - so I'd have to shade one of the implementations to be able to load both jars

tarzanek · 2020-01-13T09:30:58Z

@amnonh @elcallio
so I have a new plan based on the talks we had and my experiments
(the move of mbeans to be used directly by nodetool didn't really work, standalone fake mbean server inside nodetool too)

extract needed REST stuff in scylla-jmx into helper objects (so we avoid duplication of REST calls implementation) and into separate jar file(to be confirmed? the single jar might work)
both scylla-jmx and scylla-tools-java will leverage the helpers
improve rest calls - use properly typed jersey

elcallio

Right. I think having the MBean stuff in a REST nodeprobe is a bad solution (copied, shaded or otherwise). It is going across the river for water. Doing multiple transformations for no good reason.

I think an initial approach should copy the code from scylla-jmx (I hate copying as much as anyone, but still), even REST API stuff. Or just use new jersey-code, allowing for using typed objects and automatic serialization (see sstableinfo in scylla-jmx). And do the ops required at given nodeprobe function.

All code will be cleaner in the nodeprobe, because it will not use artificial transfer types limited by JMX. And maybe we can extract a shared sublayer instead.

elcallio · 2020-01-13T09:35:38Z

build.xml

+        <dependency groupId="javax.activation" artifactId="activation" version="1.1" />
+        <dependency groupId="javax.annotation" artifactId="javax.annotation-api" version="1.2" />
+        <dependency groupId="org.glassfish.jersey.bundles.repackaged" artifactId="jersey-guava" version="2.22.1" />
+


Good. Maven deps.
But why are some (also maven avail) files binaries below?

ant and local run, it still expects libraries to be copied to lib properly (so above code is only published to exported maven poms, not really used locally :-( )

Should be easy enough to use the maven fetch to add stuff to class path... Or simply add all maven deps to compile class path?

elcallio · 2020-01-13T09:35:43Z

.gitmodules

+[submodule "scylla-jmx"]
+	path = scylla-jmx
+	url = [email protected]:scylladb/scylla-jmx.git
+	branch = master


Can't we avoid submodules + build?
The cassandra build.xml has support for fetching maven dependencies. We should use this with a local maven repo (easy to set up in jenkins).

I had this talk with Pekka and all repos need to build (ideally) independently + we are not publishing maven artifacts from scylla-jmx and there is no plan for our own maven server
so I guess this could be solved by pushing all consumable artifacts from scylla-jmx to maven repos and having a proper scylla-jmx release cycle ... but since this doesn't make sense and is extra work we ended up deciding that submodule will be the easiest thing to do

You can easily have automatic maven repo generation in jenkins via the maven-repo plugin. Would literally be 5 minutes work to set up for our maven projects.

elcallio · 2020-01-13T09:39:19Z

src/java/org/apache/cassandra/tools/NodeTool.java

-        );
+        ));
+
+        if (REST) { commands.add(RESTInfo.class); } else { commands.add(Info.class); }


This is not accepable formatting.
if (...) {
...
} else {
...
}
Always braces.

tarzanek · 2020-06-22T15:33:01Z

this PR is kinda waiting for something else - I am duplicating code,
which Amnon didn't like and he is right, ideally if we could take code from scylla-jmx and reuse it
Where I got stuck is - when I move the code away from the namespace, the jmx server in scylla jmx has some issues starting.
(I had to move the code, to be able to consume it + so it won't conflict with code and classes in this repo with the classloader)

slivne · 2020-09-28T18:40:06Z

the is another issue with this - we need to support security - I am not sure we have this for the rest-api

mykaul · 2024-05-02T11:22:28Z

Should be closed, native nodetool is almost ready.

tarzanek added 3 commits November 15, 2019 16:32

use REST for nodetool info

bba4b35

fix dependencies, try to fix cfstats(need more fixes, test)

3afec56

mock JMX timer and histogram

4cbb881

tarzanek mentioned this pull request Nov 15, 2019

use REST for nodetool info #100

Closed

added support for more commands over REST:

eb38f68

nodetool compactionstats nodetool describecluster nodetool tablestats nodetool tablehistograms nodetool status nodetool info nodetool cfstats

tarzanek force-pushed the nodetool-rest branch from 6e17cd3 to eb38f68 Compare November 27, 2019 12:13

elcallio reviewed Jan 13, 2020

View reviewed changes

tarzanek marked this pull request as draft September 24, 2020 09:22

nyh mentioned this pull request Jul 26, 2023

Introduce a scylla-native nodetool scylladb/scylladb#14841

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nodetool over REST #121

Nodetool over REST #121

tarzanek commented Nov 15, 2019 •

edited

Loading

tarzanek commented Nov 16, 2019 •

edited

Loading

tarzanek commented Nov 27, 2019 •

edited

Loading

amnonh commented Nov 27, 2019

tarzanek commented Nov 27, 2019 •

edited

Loading

tarzanek commented Nov 27, 2019 •

edited

Loading

tarzanek commented Jan 13, 2020 •

edited

Loading

elcallio left a comment

elcallio Jan 13, 2020

tarzanek Jan 13, 2020

elcallio Jan 13, 2020

elcallio Jan 13, 2020

tarzanek Jan 14, 2020

elcallio Jan 14, 2020

elcallio Jan 13, 2020

tarzanek Jan 14, 2020

tarzanek commented Jun 22, 2020

slivne commented Sep 28, 2020

mykaul commented May 2, 2024

Nodetool over REST #121

Are you sure you want to change the base?

Nodetool over REST #121

Conversation

tarzanek commented Nov 15, 2019 • edited Loading

tarzanek commented Nov 16, 2019 • edited Loading

tarzanek commented Nov 27, 2019 • edited Loading

amnonh commented Nov 27, 2019

tarzanek commented Nov 27, 2019 • edited Loading

tarzanek commented Nov 27, 2019 • edited Loading

tarzanek commented Jan 13, 2020 • edited Loading

elcallio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarzanek commented Jun 22, 2020

slivne commented Sep 28, 2020

mykaul commented May 2, 2024

tarzanek commented Nov 15, 2019 •

edited

Loading

tarzanek commented Nov 16, 2019 •

edited

Loading

tarzanek commented Nov 27, 2019 •

edited

Loading

tarzanek commented Nov 27, 2019 •

edited

Loading

tarzanek commented Nov 27, 2019 •

edited

Loading

tarzanek commented Jan 13, 2020 •

edited

Loading