toppartitions: Fix toppartitions to only jmx once #250

StarostaGit · 2021-05-10T10:04:19Z

In line with previous API, nodetool called jmx's toppartitions
for each sampler separately. A better way to do this is to call
it once and then pick results for the samplers we need.

This is part of the fix for #243 and scylladb/scylladb#8459. It should be merged after scylladb/scylla-jmx#165

avikivity · 2021-05-10T10:21:25Z

src/java/org/apache/cassandra/service/StorageServiceMBean.java

@@ -673,7 +673,7 @@
    /** Sets the hinted handoff throttle in kb per second, per delivery thread. */
    public void setHintedHandoffThrottleInKB(int throttleInKB);

-    public CompositeData getToppartitions(String sampler, List<String> keyspaceFilters, List<String> tableFilters, int duration, int capacity, int count) throws OpenDataException;
+    public Map<String, CompositeData> getToppartitions(List<String> keyspaceFilters, List<String> tableFilters, int duration, int capacity, int count) throws OpenDataException;


How does this work? You changed the interface, but not the implementation?

Maybe the implementation is autogenerated, but it has to be generated from some IDL?

Implementation is in the jmx PR
meant to mark it as draft, sorry
the whole fix is composed of three PRs, we'll link them here once I describe the commits in the third one

But to answer the question more thoroughly, nodetool uses this interface which searches for the implementation of it in the jmx package

Then how do upgrades work? We'll need to make the old/new versions interoperate.

I've updated the PR desc
scylladb/scylla-jmx#165 has to be merged first into scylla-jmx, only then this can be merged as well

avikivity · 2021-05-10T10:23:09Z

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

@@ -482,11 +482,12 @@ public void repairAsync(final PrintStream out, final String keyspace, Map<String

    public Map<Sampler, CompositeData> getToppartitions(List<String> keyspaceFilters, List<String> tableFilters, int capacity, int duration, int count, List<Sampler> samplers) throws OpenDataException
    {
-        Map<Sampler, CompositeData> result = Maps.newHashMap();
+        Map<String, CompositeData> toppartitions = ssProxy.getToppartitions(keyspaceFilters, tableFilters, duration, capacity, count);


The call here doesn't mention samplers. So I assume it uses all possible samplers (what are they, read/write?)? If so, if the user asks for just one sampler, won't we do extra work, which is then discarded?

Maybe we need to pass the list of samplers down the stack.

The samplers are passed into this method, but not propagated further, contrary to how it worked before. The reason is we want to only call the jmx and therefore the scylla backend once (since it always gives us info for all samplers anyway) and only here, in the NodeProbe code, extract counters for the samplers the caller requested

Ok. If Scylla always samples everything, we can use the new approach without losing anything.

StarostaGit · 2021-05-27T07:23:30Z

@avikivity You wanted me to make this work with older version of the JMX call. Asked how old under the previous PR, but maybe it went unnoticed so reposting here

I already made it detect the absence of the new method, just have to know which method to redirect to - before this set of PRs or before the generic toppartitions API

avikivity · 2021-05-27T10:02:24Z

@avikivity You wanted me to make this work with older version of the JMX call. Asked how old under the previous PR, but maybe it went unnoticed so reposting here

I am quite lossy, ping when needed. It's annoying and I apologize.

by "older" I meant the previous version before your change.

I already made it detect the absence of the new method, just have to know which method to redirect to - before this set of PRs or before the generic toppartitions API

Isn't that sufficient? It will use the new API is present and fall back to the old one if not.

StarostaGit · 2021-05-27T10:07:52Z

no worries

@avikivity
by "older" I meant the previous version before your change.

That's what I wanted to know, great, will post the change soon

In line with previous API, nodetool called jmx's toppartitions for each sampler separately. A better way to do this is to call it once and then pick results for the samplers we need.

StarostaGit · 2021-05-27T12:32:15Z

@avikivity done, it falls back successfully when the new method is not found

bhalevy · 2021-06-16T10:38:33Z

@StarostaGit I still see 2 calls in the scylla log, e.g.:

INFO  2021-06-16 10:25:07,157 [shard 0] api - toppartitions query: #table_filters=all #keyspace_filters=all duration=10000 list_size= capacity=256
INFO  2021-06-16 10:25:17,174 [shard 0] api - toppartitions query: #table_filters=all #keyspace_filters=all duration=10000 list_size= capacity=256

I'm not exactly sure what this PR was trying to achieve but the second calls overrides the first one, i.e. nodetool toppartitions returns the stats only from the 2nd period as far as I can tell from the dtest.

bhalevy · 2021-06-16T11:27:11Z

Cc @haaawk let's get the toppartitions dtests pass.
They are failing since https://jenkins.scylladb.com/view/master/job/scylla-master/job/dtest-release/781/testReport/toppartitions_test/TestTopPartitions/test_any_of_required_parameters_is_missing/

StarostaGit · 2021-06-16T11:37:14Z

@StarostaGit I still see 2 calls in the scylla log, e.g.:
INFO  2021-06-16 10:25:07,157 [shard 0] api - toppartitions query: #table_filters=all #keyspace_filters=all duration=10000 list_size= capacity=256
INFO  2021-06-16 10:25:17,174 [shard 0] api - toppartitions query: #table_filters=all #keyspace_filters=all duration=10000 list_size= capacity=256
I'm not exactly sure what this PR was trying to achieve but the second calls overrides the first one, i.e. nodetool toppartitions returns the stats only from the 2nd period as far as I can tell from the dtest.

It had a wrong Class parameter passed for lists in the older/newer method call dispatch, hence for new ones it was still 2 calls to the method. There was a followup for that in #256 and it should be ok once it went in
Same for the dtests, let me call the BYO again

bhalevy · 2021-06-16T11:50:44Z

It had a wrong Class parameter passed for lists in the older/newer method call dispatch, hence for new ones it was still 2 calls to the method. There was a followup for that in #256 and it should be ok once it went in
Same for the dtests, let me call the BYO again

fwiw, they pass for me locally with #256

bhalevy · 2021-06-16T11:50:54Z

thanks

StarostaGit · 2021-06-16T11:52:34Z

it works locally for me as well, the PR with updated dtests is also just waiting for #256 so that the BYO job passes

bhalevy · 2021-06-16T12:00:12Z

what PR updating dtests? https://github.com/scylladb/scylla-dtest/pull/2147?

StarostaGit · 2021-06-16T12:09:59Z

what PR updating dtests? scylladb/scylla-dtest#2147?

Yes, this one

avikivity reviewed May 10, 2021

View reviewed changes

StarostaGit requested a review from amnonh May 10, 2021 11:17

StarostaGit force-pushed the toppartitions-fix branch from 24aea19 to c6aa4b0 Compare May 10, 2021 13:24

toppartitions: Fix toppartitions to only jmx once

a40e1f3

In line with previous API, nodetool called jmx's toppartitions for each sampler separately. A better way to do this is to call it once and then pick results for the samplers we need.

StarostaGit force-pushed the toppartitions-fix branch from c6aa4b0 to a40e1f3 Compare May 27, 2021 12:31

avikivity merged commit 599b236 into scylladb:master May 27, 2021

StarostaGit mentioned this pull request Jun 5, 2021

node_probe: toppartitions: Fix wrong class in getMethod #256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

toppartitions: Fix toppartitions to only jmx once #250

toppartitions: Fix toppartitions to only jmx once #250

StarostaGit commented May 10, 2021 •

edited

Loading

avikivity May 10, 2021

StarostaGit May 10, 2021

StarostaGit May 10, 2021

avikivity May 10, 2021

StarostaGit May 10, 2021

avikivity May 10, 2021

StarostaGit May 10, 2021

avikivity May 10, 2021

StarostaGit commented May 27, 2021 •

edited

Loading

avikivity commented May 27, 2021

StarostaGit commented May 27, 2021

StarostaGit commented May 27, 2021

bhalevy commented Jun 16, 2021

bhalevy commented Jun 16, 2021

StarostaGit commented Jun 16, 2021

bhalevy commented Jun 16, 2021

bhalevy commented Jun 16, 2021

StarostaGit commented Jun 16, 2021

bhalevy commented Jun 16, 2021 •

edited

Loading

StarostaGit commented Jun 16, 2021

toppartitions: Fix toppartitions to only jmx once #250

toppartitions: Fix toppartitions to only jmx once #250

Conversation

StarostaGit commented May 10, 2021 • edited Loading

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

StarostaGit commented May 27, 2021 • edited Loading

avikivity commented May 27, 2021

StarostaGit commented May 27, 2021

StarostaGit commented May 27, 2021

bhalevy commented Jun 16, 2021

bhalevy commented Jun 16, 2021

StarostaGit commented Jun 16, 2021

bhalevy commented Jun 16, 2021

bhalevy commented Jun 16, 2021

StarostaGit commented Jun 16, 2021

bhalevy commented Jun 16, 2021 • edited Loading

StarostaGit commented Jun 16, 2021

StarostaGit commented May 10, 2021 •

edited

Loading

StarostaGit commented May 27, 2021 •

edited

Loading

bhalevy commented Jun 16, 2021 •

edited

Loading