scylladb / scylla-jmx

Scylla JMX proxy
GNU Affero General Public License v3.0
29 stars 53 forks source link

sstableinfo: change the type of generation to string #213

Closed tchaikov closed 1 year ago

tchaikov commented 1 year ago

we will allow user to use UUID for the generation (identifier) of a SSTable. So to be compatible to both "long" and an "UUID", let's use "String" for the type of generation. this change requires the change on scylladb side, which should expose the generation as a string also in its "/storage_service/sstable_info" RESTful API.

tchaikov commented 1 year ago

the corresponding scylladb change is posted at https://github.com/scylladb/scylladb/pull/13834

tchaikov commented 1 year ago

@raphaelsc hi Raphael, could you please help review this change?

tchaikov commented 1 year ago

@bhalevy hi Benny, could you please advise whom shall i ping for reviewing this change?

bhalevy commented 1 year ago

@bhalevy hi Benny, could you please advise whom shall i ping for reviewing this change?

I'm not sure who owns tools/{jmx,java} nowadays... @mykaul / @DoronArazii please advise.

As for the change itself, I wonder if there needs to be a corresponding change in tools/java nodetool, around https://github.com/scylladb/scylla-tools-java/blob/eb3c43f8e069334f24d8aa9efd3cb350fe015aa7/src/java/org/apache/cassandra/tools/nodetool/SSTableInfo.java#L84-L111

Can you please test your change with nodetool sstableinfo? It looks like we don't have a dtest for it, so it would be a good opportunity to add one.

tchaikov commented 1 year ago

@bhalevy hi Benny, could you please advise whom shall i ping for reviewing this change?

I'm not sure who owns tools/{jmx,java} nowadays... @mykaul / @DoronArazii please advise.

As for the change itself, I wonder if there needs to be a corresponding change in tools/java nodetool, around https://github.com/scylladb/scylla-tools-java/blob/eb3c43f8e069334f24d8aa9efd3cb350fe015aa7/src/java/org/apache/cassandra/tools/nodetool/SSTableInfo.java#L84-L111

i explained the way how these three components interacted in https://github.com/scylladb/scylladb/pull/13834 . no need to change on nodetool's side.

Can you please test your change with nodetool sstableinfo? It looks like we don't have a dtest for it, so it would be a good opportunity to add one.

yes, the test was merged in https://github.com/scylladb/scylla-dtest/pull/3160 .

denesb commented 1 year ago

I'm not sure who owns tools/{jmx,java} nowadays... @mykaul / @DoronArazii please advise.

AFAIK, tools/{jmx,java} doesn't have their own mantainers, instead they are maintained by Scylla maintainers. Which means that maintainers have to review and merge changes to code they are completely unfamiliar with (for the most part). Hence there is reluctance to merge changes to these repos.

bhalevy commented 1 year ago

@denesb why did you close this PR?

denesb commented 1 year ago

@denesb why did you close this PR?

I merged it.

bhalevy commented 1 year ago

@denesb why did you close this PR?

I merged it.

Oops, sorry, it must be too early for me then, since I didn't see that :)