use REST for nodetool info #100

tarzanek · 2019-06-20T12:32:13Z

NOTE:
the important classes to review are RESTInfo.java and RESTNodeProbe.java (do ignore jmx/api namespace, it's 1-1 copy from scylla-jmx)
this is WIP, so proof of concept as such, we wanted to avoid JMX layer in between

when reviewing RESTNodeProbe -
"super." means it is just fallback to old jmx layer in NodeProbe

-R for command line is used temporarily so I can easily switch between new and old probe

build-test, stress-build is removed in build.xml just for me to build faster

I added classes from jmx/api/ from https://github.com/scylladb/scylla-jmx repo - can we make a dependency on it so we don't duplicate stuff?

performance wise for my small test the result is the same, but now there is no jmx layer in between
(note that we still connect to jmx for fallback commands even if -R is used!)

and last warning - API client doesn't use command line hostname+port yet, so I know this code fails when run against remote host

glommer · 2019-06-20T12:33:08Z

@penberg please let us know what you think

tarzanek · 2019-06-20T12:40:22Z

old:

[root@scylla scylla-tools-java]# time nodetool info
ID                     : 7dfdbd92-42d8-46bb-9e83-62bc83e5f694
Gossip active          : true
Thrift active          : true
Native Transport active: true
Load                   : 121.01 KB
Generation No          : 1560962206
Uptime (seconds)       : 71018
Heap Memory (MB)       : 23.06 / 247.50
Off Heap Memory (MB)   : 1.54
Data Center            : datacenter1
Rack                   : rack1
Exceptions             : 0
Key Cache              : entries 0, size 0 bytes, capacity 0 bytes, 0 hits, 0 requests, 0.000 recent hit rate, 0 save period in seconds
Row Cache              : entries 79, size 79 bytes, capacity 6.87 MiB, 189 hits, 508 requests, 0.372 recent hit rate, 0 save period in seconds
Counter Cache          : entries 0, size 0 bytes, capacity 0 bytes, 0 hits, 0 requests, 0.000 recent hit rate, 0 save period in seconds
Percent Repaired       : 0.0%
Token                  : (invoke with -T/--tokens to see all 256 tokens)

real    0m11.086s
user    0m6.432s
sys     0m0.353s

new:

[root@scylla scylla-tools-java]# time ./bin/nodetool -R info
ID                     : 7dfdbd92-42d8-46bb-9e83-62bc83e5f694
Gossip active          : true
Thrift active          : true
Native Transport active: true
Load                   : 121.01 KB
Generation No          : 1560962206
Uptime (seconds)       : 0
Heap Memory (MB)       : 0.00 / 0.00
Off Heap Memory (MB)   : 1.54
Data Center            : datacenter1
Rack                   : rack1
Exceptions             : 0
Key Cache              : entries 0, size 0 bytes, capacity 0 bytes, 0 hits, 0 requests, 0.000 recent hit rate, 0 save period in seconds
Row Cache              : entries 79, size 79 bytes, capacity 6.87 MiB, 187 hits, 506 requests, 0.370 recent hit rate, 0 save period in seconds
Counter Cache          : entries 0, size 0 bytes, capacity 0 bytes, 0 hits, 0 requests, 0.000 recent hit rate, 0 save period in seconds
Percent Repaired       : 0.0%
Token                  : (invoke with -T/--tokens to see all 256 tokens)

real    0m13.849s
user    0m10.807s
sys     0m2.043s

I tested with -T and it works well, too
uptime and heaps are 0 in new version due to #98

build.xml

penberg · 2019-06-20T12:46:34Z

Some high-level comments:

The approach makes tons of sense, and it's something we have discussed internally before too.

We should indeed make the REST API parts of Scylla JMX available as a library for this effort. One option is to make a separate "scylla-api-java.git", which has the generally useful bits. @avikivity, what do you think?

penberg · 2019-06-20T12:48:15Z

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

        List<Class<? extends Runnable>> commands = asList(
                Help.class,
-                Info.class,
+                REST==true?RESTInfo.class:Info.class,


Perhaps it would be simpler to always have Info.class here and make that class internally switch between RESTInfo and JMXInfo?

this is a good comment ... I wanted to avoid modifying Info class as we consume it from upstream - so if we will merge upstream, this approach(to modify loaded classes) should be easier to handle conflicts and we will know what should be ported where

Sure, it makes sense not to modify Info class directly, but what if we add a InfoWrapper class that we use here, which delegates to Info or RESTInfo?

penberg · 2019-06-20T12:49:36Z

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

        @Option(type = OptionType.GLOBAL, name = {"-pwf", "--password-file"}, description = "Path to the JMX password file")
        private String passwordFilePath = EMPTY;

+        @Option(type = OptionType.GLOBAL, name = {"-R"}, description = "Use REST instead of JMX")


Maybe elaborate more in the description. For example, something like this:

Use Scylla's REST API (when supported) instead of the Cassandra-compatible JMX protocol for operations

we actually want to get rid of this option :-)
I have it there to ease my testing and comparing if I miss functionality from old NodeProbe

I would like to keep the option (and have it disabled by default) for one or two releases. But we certainly can switch over to REST altogether once it's proven stable in production.

This will get confusing when we default to it.
Pekka, if you want an option here, I'd like to suggest something like --probe=[rest|jmx] or --proto so we can choose any, and not only an enable-style flag so we can switch between both

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

penberg · 2019-06-20T12:53:07Z

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

+//        } catch (UnknownHostException e) {
+//            e.printStackTrace(); //TODO fix DNS name lookup error
+//        }
+//        return client.getStringValue("/snitch/datacenter", queryParams, 10000);


I guess the commented out code will be removed from the final pull request.

correct, this is a very early draft, so no cleanup was done

tarzanek · 2019-06-20T12:55:38Z

I force pushed new update, hopefully it still keeps some history, seems comments are still here :-)
I got rid of -R
and added few jars that are needed for this code to work (I will need to check ALL of the jar dependencies to make sure it works 100%, since scylla-jmx is on mvn and this repo is on ant :-( )

penberg · 2019-06-20T12:55:58Z

Overall, the pull request makes sense to me. I think RESTNodeProbe can be simplified and cleaned up, and, as you also say, we need to figure out a way to eliminate APIClient duplication between JMX and this repository. Other than that, I think we can definitely go ahead with this approach.

tarzanek · 2019-06-21T06:19:14Z

@haaawk @elcallio could give your insights on the approach too?
I basically have only one concern, which is data that should be cached (jmx now caches few things, I think snitch, datacenter and rack and few metrics for few seconds), will loose caching now, since nodetool will re-query it always.
(which could in the end prove more useful than jmx refreshing it always :-) )

elcallio · 2019-06-24T07:58:52Z

@tarzanek I am generally for this, though we have not done this earlier because (apparently?) there are customers relying on JMX supprt (not for nodetool, but third party stuff).
However, I really feel you need to make scylla-jmx-rest-api junk a proper dependency, not just copied binaries. As suggested above, a library split and git submodule is one approach. Another is to use the maven-copy code in build.xml and extend it to pick scylla libraries and dependencies as well. This is probably the simpler approach right now. (We should add maven repo functionality to our jenkins so versioned scylla-jmx can be copied from there).

tarzanek · 2019-06-24T12:59:10Z

piotr [2:57 PM]
flag shouldn't be binary
but rather take value of communication method
not -use-rest

but -protocol=rest
-protocol=jmx

elcallio · 2019-06-24T13:45:34Z

Btw, before we can even think of making nodetool REST-only, the REST api needs to support HTTPS/TLS. JMX connectivity can be secured on Java level, and scylla-jmx to localhost w.o. security is probably fine, but for remote client to REST api, both SSL + auth is really needed.

glommer · 2019-06-25T13:50:57Z

@tarzanek I am generally for this, though we have not done this earlier because (apparently?) there are customers relying on JMX supprt (not for nodetool, but third party stuff).
However, I really feel you need to make scylla-jmx-rest-api junk a proper dependency, not just copied binaries. As suggested above, a library split and git submodule is one approach. Another is to use the maven-copy code in build.xml and extend it to pick scylla libraries and dependencies as well. This is probably the simpler approach right now. (We should add maven repo functionality to our jenkins so versioned scylla-jmx can be copied from there).

Some background here: Indeed we won't be able to retire scylla-jmx by doing this, since there are other consumers (even when we finish the conversion). However we're seeing scylla-jmx slow down nodetool in some queries, and even the people who need no other API are paying the price. The goal, at least at first, is to convert nodetool, and then be able to disable scylla-jmx (or just leave it there, running doing nothing) for the customers who don't need it.

tarzanek · 2019-07-12T11:53:55Z

I addressed all the comments, however I lack a proper packaging test (so far I was running this from my workspace dir and only tried the artifact target)

tarzanek · 2019-07-12T12:15:30Z

http://jenkins.scylladb.com/view/master/job/scylla-master/job/manual-and-scheduled-tests/job/byo_build_tests_dtest/437/
runs as a package test now

tarzanek · 2019-07-12T15:36:44Z

ok, packaging tests failed after packaging, I lack host and port support for REST, fixing now,
rebuilding http://jenkins.scylladb.com/view/master/job/scylla-master/job/manual-and-scheduled-tests/job/byo_build_tests_dtest/442/

amnonh · 2019-07-23T14:35:02Z

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

        @Option(type = OptionType.GLOBAL, name = {"-p", "--port"}, description = "Remote jmx agent port number")
        private String port = "7199";

+        @Option(type = OptionType.GLOBAL, name = {"-o", "--restport"}, description = "Remote Scylla REST port number")


nitpicking short and long version should at least attempt to be similar

amnonh · 2019-07-23T14:35:08Z

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

        @Option(type = OptionType.GLOBAL, name = {"-pwf", "--password-file"}, description = "Path to the JMX password file")
        private String passwordFilePath = EMPTY;

+        @Option(type = OptionType.GLOBAL, name = {"-r", "--protocol"}, description = "Use rest(default, only for what is ported) or jmx(legacy) protocol")


nitpicking again, the -r is not a great short version of --protocol

amnonh · 2019-07-23T14:36:49Z

src/java/org/apache/cassandra/tools/nodetool/RESTInfo.java

+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */


I think we should add a scylla clause

A general comment, why do we need to replace the Info class at all?

If we replace the probe part, the rest should be transparent, right?
This way it will be easier to follow upstream changes

the problem is that the probe methods use mxbeans for DAOs, so this is to get rid of mxbeans in REST part completely without changing much of original classes
obviously this creates later an update problem if we will sync with upstream(but the problem will be there anyways), but it will get us rid of mxbeans and merges with upstream should be easier to sync with modified classes(that use the mxbeans methods from probe)

amnonh · 2019-07-23T14:44:05Z

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

+/**
+ * REST client operations for Scylla.
+ */
+public class RESTNodeProbe extends NodeProbe


For testing during the transition phase, we can use an empty RESTNodeProbe and gradually add override methods.
What we don't override will be taken from NodeProbe

I am calling super() now to parent jmx so fallback is to jmx where not implemented.
Are you suggesting to just return empty values instead?

amnonh · 2019-07-24T15:54:18Z

an alternative (I'm not sure what the code impact would be) to create a fake jmx implementation layer, add the scylla-jmx classes inside the nodetool so nodeprobe would use it directly.

We can probably do that by changing the getter function on NodeProbe

tarzanek · 2019-07-26T09:51:39Z

an alternative (I'm not sure what the code impact would be) to create a fake jmx implementation layer, add the scylla-jmx classes inside the nodetool so nodeprobe would use it directly.

We can probably do that by changing the getter function on NodeProbe

interesting - so all the jmx mxbean code building would happen in one jvm - which is actually what we want to get rid of (it grows too much and is too slow)
how it would however happen in one JVM is a question, I can try if I will have spare time, but until then current approach seems just more obvious (unless somewhere the mxbean caching has better response than querying REST API layer) - I still expect the jmx to be just too slow

amnonh · 2019-07-26T12:41:28Z

On Friday, July 26, 2019, Lubos Kosco ***@***.***> wrote: an alternative (I'm not sure what the code impact would be) to create a fake jmx implementation layer, add the scylla-jmx classes inside the nodetool so nodeprobe would use it directly. We can probably do that by changing the getter function on NodeProbe interesting - so all the jmx mxbean code building would happen in one jvm - which is actually what we want to get rid of (it grows too much and is too slow) how it would however happen in one JVM is a question, I can try if I will have spare time, but until then current approach seems just more obvious (unless somewhere the mxbean caching has better response than querying REST API layer)

I think that without the jmx part it would work faster. I've looked at node probe code and it's quite straightforward. The big advantage is that there will be no code replication and minimal dependency between our code and cassandra —

…

You are receiving this because you commented. Reply to this email directly, view it on GitHub <#100?email_source=notifications&email_token=AAQFDPZG3SJKMUWBROM222LQBLCK3A5CNFSM4HZSQ4O2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD24C5JQ#issuecomment-515387046>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQFDP7NXY5PQS3MPI7AUF3QBLCK3ANCNFSM4HZSQ4OQ> .

tarzanek · 2019-11-15T15:36:11Z

closing in favour of #121

penberg reviewed Jun 20, 2019

View reviewed changes

build.xml Outdated Show resolved Hide resolved

penberg reviewed Jun 20, 2019

View reviewed changes

src/java/org/apache/cassandra/tools/RESTNodeProbe.java Show resolved Hide resolved

penberg reviewed Jun 20, 2019

View reviewed changes

src/java/org/apache/cassandra/tools/RESTNodeProbe.java Show resolved Hide resolved

penberg reviewed Jun 20, 2019

View reviewed changes

tarzanek force-pushed the master branch from 64755ef to a29d781 Compare June 20, 2019 12:53

tarzanek force-pushed the master branch from a29d781 to c4b6c98 Compare July 12, 2019 11:51

tarzanek changed the title ~~WIP - use REST for nodetool info~~ use REST for nodetool info Jul 12, 2019

tarzanek force-pushed the master branch from c4b6c98 to fba7500 Compare July 12, 2019 15:44

use REST for nodetool info

5e3a708

tarzanek force-pushed the master branch from fba7500 to 5e3a708 Compare July 12, 2019 16:44

add REST node port cli option

c65e774

amnonh reviewed Jul 23, 2019

View reviewed changes

tarzanek mentioned this pull request Aug 21, 2019

Use IPs instead of localhost for nodetool commands scylladb/scylla-ccm#170

Merged

tarzanek changed the base branch from master to next August 21, 2019 09:14

tarzanek mentioned this pull request Sep 9, 2019

support for nodetool REST scylladb/scylla-ccm#174

Closed

tarzanek closed this Nov 15, 2019

tarzanek mentioned this pull request Jan 13, 2020

Nodetool over REST #121

Draft

tarzanek mentioned this pull request Dec 3, 2020

nodetool reports the uptime of scylla-jmx and not the uptime of scylla server #98

Open

use REST for nodetool info #100

use REST for nodetool info #100

Uh oh!

Conversation

tarzanek commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glommer commented Jun 20, 2019

Uh oh!

tarzanek commented Jun 20, 2019

Uh oh!

Uh oh!

penberg commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarzanek commented Jun 20, 2019

Uh oh!

penberg commented Jun 20, 2019

Uh oh!

tarzanek commented Jun 21, 2019

Uh oh!

elcallio commented Jun 24, 2019

Uh oh!

tarzanek commented Jun 24, 2019

Uh oh!

elcallio commented Jun 24, 2019

Uh oh!

glommer commented Jun 25, 2019

Uh oh!

tarzanek commented Jul 12, 2019

Uh oh!

tarzanek commented Jul 12, 2019

Uh oh!

tarzanek commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amnonh commented Jul 24, 2019

Uh oh!

tarzanek commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amnonh commented Jul 26, 2019 via email

Uh oh!

tarzanek commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

tarzanek commented Jun 20, 2019 •

edited

Loading

tarzanek commented Jul 12, 2019 •

edited

Loading

tarzanek commented Jul 26, 2019 •

edited

Loading