CASSANDRA-16095 - Nodetool tablestats WriteLatency error

[dcapwell]

No description provided.

[dcapwell]

Pulled from https://issues.apache.org/jira/browse/CASSANDRA-16213, was going to add them there but need them in this patch as well, so copy/pasted from there (but added docs for the ones missing)

[yifan-c]

The get with overrideDefaultValue methods are not used. Not a big problem if the other patch referenced is to be merged.

[dcapwell]

renamed to all caps so its more clear this is global and not local to the table; seems that this naming convention caused all to be confused with local table names rather than global metrics, so switch to java naming convention to help to prevent this next time.

[dcapwell]

did this in https://issues.apache.org/jira/browse/CASSANDRA-16213 as its useful for debugging.

[dcapwell]

this was a regressions... when not using gossip we didn't register the distributed tables, and we tried to force trace regardless...

right before we gossip tokens around we call this method, so adding it here makes sure the distributed tables work with and without gossip.

[jonmeredith]

LGTM.

Is it worth also checking the KeyspaceMetrics are created and removed and that dropping a Keyspace removes all contained table metrics?

[jonmeredith]

Could you use org.apache.cassandra.db.SystemKeyspace#ALL_TABLE_NAMES here? and similarly create ALL_TABLE_NAMES for the other keyspaces where they're missing?

[dcapwell]

I didn't want to as that would load the whole DB, I could refactor to avoid that but felt simplest to just fork.

[dcapwell]

I don't see that in 3.0 or trunk...

$ grep -r ALL_TABLE_NAMES src/java/
$

[jonmeredith]

My mistake :)

[yifan-c]

I think it is better to load the tables programmatically. So compiler prompts when code changes. It can be done via

        Map<String, Collection<String>> systemTables = new HashMap<>();
        Consumer<KeyspaceMetadata> loader = meta ->
        {
            Set<String> tables = meta.tables.stream().map(t -> t.name).collect(Collectors.toSet());
            systemTables.put(meta.name, tables);
        };
        Arrays.asList(SystemKeyspace.metadata(), AuthKeyspace.metadata(), SystemDistributedKeyspace.metadata(),
                      SchemaKeyspace.metadata(), TraceKeyspace.metadata())
              .forEach(loader);

[dcapwell]

that has the same issue, we load the whole DB into the app space, so was trying to avoid that.

[dcapwell]

I switched to doing this, I do this by loading all tables in the first instance.

[dcapwell]

LGTM.

Is it worth also checking the KeyspaceMetrics are created and removed and that dropping a Keyspace removes all contained table metrics?

I thought about that but was lazy!

[dcapwell]

I updated to also check the keyspace metric, there isn't a keyspace mbean to validate, so only look for the keyspace version of the metrics list

[jonmeredith]

Thanks for the extra tests.

[yifan-c]

Looks good overall.

[yifan-c]

The get with overrideDefaultValue methods are not used. Not a big problem if the other patch referenced is to be merged.

[yifan-c]

great to group the static variable together.

[yifan-c]

nit: maybe also create constants for the metrics names? since this patch is doing some code cleanup already.
(Maybe too much work. your call)

[yifan-c]

I think it is better to load the tables programmatically. So compiler prompts when code changes. It can be done via

        Map<String, Collection<String>> systemTables = new HashMap<>();
        Consumer<KeyspaceMetadata> loader = meta ->
        {
            Set<String> tables = meta.tables.stream().map(t -> t.name).collect(Collectors.toSet());
            systemTables.put(meta.name, tables);
        };
        Arrays.asList(SystemKeyspace.metadata(), AuthKeyspace.metadata(), SystemDistributedKeyspace.metadata(),
                      SchemaKeyspace.metadata(), TraceKeyspace.metadata())
              .forEach(loader);