Re: [PR] CASSANDRA-21605 Improve algorithmic complexity for finding intersecting SSTables for LCS compaction candidates [cassandra]

["gandherva (via GitHub)" <gi...@apache.org>]

gandherva commented on code in PR #5061:
URL: https://github.com/apache/cassandra/pull/5061#discussion_r4244405035


##########
src/java/org/apache/cassandra/db/compaction/LeveledGenerations.java:
##########
@@ -96,6 +96,13 @@ Set<SSTableReader> get(int level)
         return levels[level - 1];
     }
 
+    TreeSet<SSTableReader> getSortedLevel(int level)
+    {

Review Comment:
   (1) There is already `get(level)` and the new `getSortedLevel`, is a copy of `get(level)` with one additional check that level should be greater than 0. 
   
   (2) Having two methods one as `get(level)` and one as `getSortedlevel()` could mislead readers to think that the 2nd method would give something different while both return the same set. 
   
   So for these two reasons, I will suggest dropping `getSortedLevel()` method and doing one of these options. 
   
   Option A:  As the only validation which is needed is level > 0 , this could be performed by the caller before calling get(level) and casting it to a Tree set. In the case of the current caller as it is level +1 before calling, may be the validation is not needed.  I will leave it upto you. 
   
   Option B: Alternatively if we want to have a method for this, we could create one called `getNonL0Level`  or `getNonZeroLevel` , inside that method we could do a check for level not 0 an then call `get(level)` so that it handles the rest of validations. 
   
   Both these options avoid duplication of code and avoid creation of a potentially misleading method called getSortedLevel which is almost a copy of get(level)
   



##########
src/java/org/apache/cassandra/db/compaction/LeveledManifest.java:
##########
@@ -592,6 +596,30 @@ private Collection<SSTableReader> getCandidatesFor(int level)
         return Collections.emptyList();
     }
 
+    /**
+     * Precondition: SSTables in sstablesNextLevel must be disjoint
+    */
+    @VisibleForTesting
+    protected static Set<SSTableReader> getIntersectingSSTablesFromTreeSet(SSTableReader sstable, TreeSet<SSTableReader> sstablesNextLevel)
+    {
+        Set<SSTableReader> candidates = new HashSet<>();
+
+        if (sstablesNextLevel.isEmpty())
+            return candidates;
+
+        SSTableReader start = sstablesNextLevel.floor(sstable);
+        Iterator<SSTableReader> it = sstablesNextLevel.tailSet(start != null ? start : sstablesNextLevel.first(), true).iterator();

Review Comment:
   To improve readability and to avoid calling 'sstablesNextLevel.tailSet(sstablesNextLevel.first())', as we are calling tailSet with the first element which does extra work(an additional tree navigation to find the first element and creating a view object and its sub objects) that could be avoided by using TreeSet directly
   
   I'd suggest to change it like this 
   
   if start is null then do 
   `it = sstablesNextLevel.iterator()`
   or else do `it =  sstablesNextLevel.tailSet(start,true).iterator() `
   
   This will also avoid the need to do the early return with `if (sstablesNextLevel.isEmpty())` check  at line 607-608. So we can remove them as well. 
   



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org