adelapena commented on a change in pull request #1150:
URL: https://github.com/apache/cassandra/pull/1150#discussion_r706308443
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java
##########
@@ -137,7 +146,87 @@ public void cleanUp()
}
@Test
- public void testSingleExpressionQueries()
+ public void testSASIComponentsAddedToSnapshot() throws Throwable
+ {
+ String snapshotName = "sasi_test";
+ Map<String, Pair<String, Integer>> data = new HashMap<>();
+ Random r = new Random();
+
+ for (int i = 0; i < 100; i++)
+ {
+ data.put(UUID.randomUUID().toString(),
Pair.create(UUID.randomUUID().toString(), r.nextInt()));
+ }
+
+ ColumnFamilyStore store = loadData(data, true);
+ store.forceMajorCompaction();
+
+ try
+ {
+ // left holds table component sizes, right holds total size of
index components (SI_*)
+ Pair<Long, Long> tableIndexSizes =
takeSnapshotAndCheckComponents(store, snapshotName);
+ Map<String, Pair<Long, Long>> details = store.getSnapshotDetails();
+
+ // check that SASI components are included in the computation of
snapshot size
+ Assert.assertEquals((long) details.get("sasi_test").right,
tableIndexSizes.left + tableIndexSizes.right);
+ }
+ finally
+ {
+ store.clearSnapshot("sasi_test");
+ }
+ }
+
+ private Pair<Long, Long> takeSnapshotAndCheckComponents(ColumnFamilyStore
cfs, String snapshotName) throws Throwable
+ {
+ Set<SSTableReader> ssTableReaders = cfs.getLiveSSTables();
+ Set<Component> sasiComponents = new HashSet<>();
+ for (Index index : cfs.indexManager.listIndexes())
+ {
+ if (index instanceof SASIIndex)
+ sasiComponents.add(((SASIIndex)
index).getIndex().getComponent());
+ }
+
+ cfs.snapshot(snapshotName);
+ JSONObject manifest = (JSONObject) new JSONParser().parse(new
FileReader(cfs.getDirectories().getSnapshotManifestFile(snapshotName)));
+ JSONArray files = (JSONArray) manifest.get("files");
+ Assert.assertEquals(ssTableReaders.size(), files.size());
+ Map<Descriptor, Set<Component>> snapshots =
cfs.getDirectories().sstableLister(Directories.OnTxnErr.IGNORE).snapshots(snapshotName).list();
+
+ long indexSize = 0;
+ long tableSize = 0;
+
+ for (SSTableReader sstable : ssTableReaders)
+ {
+
+ File snapshotDirectory =
Directories.getSnapshotDirectory(sstable.descriptor, snapshotName);
+ Descriptor tmp = new Descriptor(snapshotDirectory,
+ sstable.getKeyspaceName(),
+ sstable.getColumnFamilyName(),
+ sstable.descriptor.generation,
+ sstable.descriptor.formatType);
+
+ Set<Component> components = snapshots.get(tmp);
+
+
+ Assert.assertNotNull(components);
+ Assert.assertTrue(components.containsAll(sasiComponents));
+
+ for (Component c : components)
+ {
+ Path p = Paths.get(sstable.descriptor + "-" + c.name);
+ long size = Files.size(p);
+ if (c.name.contains("SI_")) {
+ indexSize += size;
+ } else {
+ tableSize += size;
+ }
Review comment:
Nit: braces placement should be:
```suggestion
if (c.name.contains("SI_"))
{
indexSize += size;
}
else
{
tableSize += size;
}
```
Or without braces, if you prefer so:
```suggestion
if (c.name.contains("SI_"))
indexSize += size;
else
tableSize += size;
```
##########
File path: src/java/org/apache/cassandra/index/sasi/conf/DataTracker.java
##########
@@ -77,6 +77,11 @@ public View getView()
}
while (!view.compareAndSet(currentView, newView));
+ for(SSTableReader sstable: indexedSSTables)
Review comment:
Nit: missed whitespaces:
```suggestion
for (SSTableReader sstable : indexedSSTables)
```
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java
##########
@@ -137,7 +146,87 @@ public void cleanUp()
}
@Test
- public void testSingleExpressionQueries()
+ public void testSASIComponentsAddedToSnapshot() throws Throwable
+ {
+ String snapshotName = "sasi_test";
+ Map<String, Pair<String, Integer>> data = new HashMap<>();
+ Random r = new Random();
+
+ for (int i = 0; i < 100; i++)
+ {
+ data.put(UUID.randomUUID().toString(),
Pair.create(UUID.randomUUID().toString(), r.nextInt()));
+ }
+
+ ColumnFamilyStore store = loadData(data, true);
+ store.forceMajorCompaction();
+
+ try
+ {
+ // left holds table component sizes, right holds total size of
index components (SI_*)
+ Pair<Long, Long> tableIndexSizes =
takeSnapshotAndCheckComponents(store, snapshotName);
+ Map<String, Pair<Long, Long>> details = store.getSnapshotDetails();
+
+ // check that SASI components are included in the computation of
snapshot size
+ Assert.assertEquals((long) details.get("sasi_test").right,
tableIndexSizes.left + tableIndexSizes.right);
+ }
+ finally
+ {
+ store.clearSnapshot("sasi_test");
+ }
+ }
+
+ private Pair<Long, Long> takeSnapshotAndCheckComponents(ColumnFamilyStore
cfs, String snapshotName) throws Throwable
Review comment:
Do we need to have this in a separate method, with a single caller, two
separate responsibilities and a composite return type? Also the test doesn't
actually check the components, it only collects the sizes. I think that at
least the snapshotting (`cfs.snapshot(snapshotName)`) could be done out of the
method, and maybe the entire method could be inlined in the caller test. Anyway
I don't have a strong opinion about this, just ignore the suggestion if you
don't agree.
##########
File path: test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java
##########
@@ -137,7 +146,87 @@ public void cleanUp()
}
@Test
- public void testSingleExpressionQueries()
+ public void testSASIComponentsAddedToSnapshot() throws Throwable
Review comment:
Nice test :)
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
