ekaterinadimitrova2 commented on a change in pull request #856:
URL: https://github.com/apache/cassandra/pull/856#discussion_r559843766
##########
File path: src/java/org/apache/cassandra/db/compaction/CompactionManager.java
##########
@@ -1476,7 +1472,7 @@ public static SSTableWriter
createWriterForAntiCompaction(ColumnFamilyStore cfs,
* @param txn a transaction over the repaired sstables to anticompact
* @param ranges full and transient ranges to be placed into one of the
new sstables. The repaired table will be tracked via
* the {@link
org.apache.cassandra.io.sstable.metadata.StatsMetadata#pendingRepair} field.
- * @param sessionID the repair session we're anti-compacting for
+ * @param pendingRepair the repair session we're anti-compacting for
Review comment:
Good catch!
##########
File path:
src/java/org/apache/cassandra/tools/nodetool/stats/TableStatsHolder.java
##########
@@ -35,13 +35,15 @@
public final boolean humanReadable;
public final String sortKey;
public final int top;
+ public final boolean boundaryCheck;
- public TableStatsHolder(NodeProbe probe, boolean humanReadable, boolean
ignore, List<String> tableNames, String sortKey, int top)
+ public TableStatsHolder(NodeProbe probe, boolean humanReadable, boolean
ignore, List<String> tableNames, String sortKey, int top, boolean boundaryCheck)
{
this.keyspaces = new ArrayList<>();
this.humanReadable = humanReadable;
this.sortKey = sortKey;
this.top = top;
+ this.boundaryCheck = boundaryCheck;
Review comment:
nit:I would add an empty line here
##########
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:
Shouldn't it be `return !diskBoundaries.isMisplaced(sstable,
currentDirectory);`?
My reading is that if it is in the correct location it won't be misplaced.
Also, I see you do the opposite check in diskBoundaries.isMisplaced
We don't have it tested in the unit tests as far as I can see, that is why
it didn't pop up as an issue I think
##########
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:
Code-wise boundary-check makes sense as an option name but do you think
for the users it will be intuitive until they read the description? I don't
have immediate suggestion but I will put a bit more thinking on this one.
##########
File path:
test/unit/org/apache/cassandra/tools/nodetool/stats/NodetoolTableStatsTest.java
##########
@@ -237,4 +240,20 @@ public void testTopArg()
tool.assertCleanStdErr();
assertEquals(1, tool.getExitCode());
}
+
+ @Test
+ public void testBoundaryCheckArg()
+ {
+ Arrays.asList("-B", "--boundary-check").forEach(arg -> {
+ ToolResult tool = ToolRunner.invokeNodetool("tablestats", arg,
"system.local");
+ assertEquals("Arg: [" + arg + "]",
StringUtils.countMatches(tool.getStdout(), "SSTables in correct location: "),
1);
+ assertTrue("Arg: [" + arg + "]",
tool.getCleanedStderr().isEmpty());
+ assertEquals("Arg: [" + arg + "]", 0, tool.getExitCode());
Review comment:
nit:I know it is everywhere in this class but why don't we correct "]"
to ']'?
##########
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:
Is there a rule when should we use capital letter and when not? I see
both and if there is no rule I would leave it lower-case as more intuitive,
maybe?
##########
File path:
test/distributed/org/apache/cassandra/distributed/test/MultipleDataDirectoryTest.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.
+ */
+
+package org.apache.cassandra.distributed.test;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.Iterator;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.db.ColumnFamilyStore;
+import org.apache.cassandra.db.Keyspace;
+import org.apache.cassandra.distributed.Cluster;
+import org.apache.cassandra.distributed.api.IInvokableInstance;
+import org.apache.cassandra.io.sstable.Component;
+import org.apache.cassandra.io.sstable.Descriptor;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.io.util.FileUtils;
+
+public class MultipleDataDirectoryTest extends TestBaseImpl
+{
+ private static Cluster CLUSTER;
+ private static IInvokableInstance NODE;
+
+ @BeforeClass
+ public static void before() throws IOException
+ {
+ CLUSTER =
init(Cluster.build().withNodes(1).withDataDirCount(3).start());
+ NODE = CLUSTER.get(1);
+ CLUSTER.schemaChange(withKeyspace("CREATE TABLE %s.cf (k text, c1
text, c2 text, PRIMARY KEY (k)) WITH compaction = {'class':
'LeveledCompactionStrategy', 'enabled': 'false'}"));
+ Assert.assertEquals(3, NODE.callsOnInstance(() ->
DatabaseDescriptor.getAllDataFileLocations().length).call().intValue());
+ }
+
+ @AfterClass
+ public static void after()
+ {
+ if (CLUSTER != null)
+ CLUSTER.close();
+ }
+
+ @Before
+ public void populateData()
+ {
+ final int rowsPerFile = 500;
+ final int files = 5;
+ for (int k = 0; k < files; k++)
+ {
+ for (int i = k * rowsPerFile; i < k * rowsPerFile + rowsPerFile;
++i)
+ NODE.executeInternal(withKeyspace("INSERT INTO %s.cf (k, c1,
c2) VALUES (?, 'value1', 'value2');"), Integer.toString(i));
+ NODE.nodetool("flush");
+ }
+ }
+
+ @After
+ public void cleanupData()
+ {
+ NODE.runOnInstance(() -> {
+
Keyspace.open(KEYSPACE).getColumnFamilyStore("cf").truncateBlocking();
+ });
+ }
+
+ @Test
+ public void testSSTablesAreInCorrectLocation()
+ {
+ NODE.runOnInstance(() -> {
+ ColumnFamilyStore cfs =
Keyspace.open(KEYSPACE).getColumnFamilyStore("cf");
+ Assert.assertFalse("All SSTables should be in the correct
location",
+ cfs.isSSTableMisplaced());
+ });
+ }
+
+ @Test
+ public void testDetectSSTableMisplaced()
+ {
+ NODE.runOnInstance(() -> {
+ ColumnFamilyStore cfs =
Keyspace.open(KEYSPACE).getColumnFamilyStore("cf");
+ Assert.assertNotEquals(0, cfs.getLiveSSTables().size());
+ Iterator<SSTableReader> sstables =
cfs.getLiveSSTables().iterator();
+ // finding 2 descriptors that live in different data directory
+ Descriptor first = sstables.next().descriptor;
+ Descriptor second = null;
+ while (sstables.hasNext() && second == null) {
+ second = sstables.next().descriptor;
+ if (first.directory.equals(second.directory))
+ second = null;
+ }
+ Assert.assertNotNull("There should be SSTables in multiple data
directories", second);
+ second = cfs.newSSTableDescriptor(second.directory); // getting a
new file index in order to move SSTable between directories.
Review comment:
nit:I would move the comment to a separate line
##########
File path:
test/distributed/org/apache/cassandra/distributed/test/MultipleDataDirectoryTest.java
##########
@@ -0,0 +1,135 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * 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.
+ */
+
+package org.apache.cassandra.distributed.test;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.util.Arrays;
Review comment:
Unused imports:
import java.util.Arrays;
import org.apache.cassandra.io.util.FileUtils;
----------------------------------------------------------------
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]
