yifan-c commented on a change in pull request #856:
URL: https://github.com/apache/cassandra/pull/856#discussion_r561608058
##########
File path: src/java/org/apache/cassandra/tools/nodetool/TableStats.java
##########
@@ -68,6 +68,11 @@
description = "Show only the top K tables for the sort key
(specify the number K of tables to be shown")
private int top = 0;
+ @Option(title = "boundary_check",
+ name = {"-B", "--boundary-check"},
Review comment:
Good point. It is probably not be very intuitive. Maybe call it
`--sstable-location-check` (short form as `-L`)
##########
File path:
test/unit/org/apache/cassandra/tools/nodetool/stats/NodetoolTableStatsTest.java
##########
@@ -78,11 +78,14 @@ public void testMaybeChangeDocs()
" [(-pp | --print-port)] [(-pw
<password> | --password <password>)]\n" +
" [(-pwf <passwordFilePath> |
--password-file <passwordFilePath>)]\n" +
" [(-u <username> | --username
<username>)] tablestats\n" +
- " [(-F <format> | --format <format>)]
[(-H | --human-readable)] [-i]\n" +
- " [(-s <sort_key> | --sort <sort_key>)]
[(-t <top> | --top <top>)] [--]\n" +
- " [<keyspace.table>...]\n" +
+ " [(-B | --boundary-check)] [(-F
<format> | --format <format>)]\n" +
+ " [(-H | --human-readable)] [-i] [(-s
<sort_key> | --sort <sort_key>)]\n" +
+ " [(-t <top> | --top <top>)] [--]
[<keyspace.table>...]\n" +
"\n" +
- "OPTIONS\n" +
+ "OPTIONS\n" +
+ " -B, --boundary-check\n" +
Review comment:
Not sure if there is a rule. The nodetool options naming does not seem
to be very consistent among all the commands.
I do not have preferences. Lower-case sounds good to me. It is a more common
pattern for general cli.
##########
File path: src/java/org/apache/cassandra/db/compaction/CompactionManager.java
##########
@@ -660,13 +660,9 @@ private boolean inCorrectLocation(SSTableReader sstable)
if (!cfs.getPartitioner().splitter().isPresent())
return true;
- int diskIndex = diskBoundaries.getDiskIndex(sstable);
- PartitionPosition diskLast =
diskBoundaries.positions.get(diskIndex);
-
- // the location we get from directoryIndex is based on the
first key in the sstable
- // now we need to make sure the last key is less than the
boundary as well:
- Directories.DataDirectory dataDirectory =
cfs.getDirectories().getDataDirectoryForFile(sstable.descriptor);
- return
diskBoundaries.directories.get(diskIndex).equals(dataDirectory) &&
sstable.last.compareTo(diskLast) <= 0;
+ // Compare the expected data directory for the sstable with
its current data directory
+ Directories.DataDirectory currentDirectory =
cfs.getDirectories().getDataDirectoryForFile(sstable.descriptor);
+ return diskBoundaries.isMisplaced(sstable, currentDirectory);
Review comment:
👍 good catch!
I shall add the tests too.
##########
File path: src/java/org/apache/cassandra/tools/nodetool/TableStats.java
##########
@@ -68,6 +68,11 @@
description = "Show only the top K tables for the sort key
(specify the number K of tables to be shown")
private int top = 0;
+ @Option(title = "boundary_check",
+ name = {"-B", "--boundary-check"},
Review comment:
Good point. It is probably not very intuitive. Maybe call it
`--sstable-location-check` (short form as `-L`)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
