Re: [PR] CASSANDRA-21485 Keep Auto Repair byte estimates consistent [cassandra]

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

bbotella commented on code in PR #5216:
URL: https://github.com/apache/cassandra/pull/5216#discussion_r4154977710


##########
src/java/org/apache/cassandra/repair/autorepair/AutoRepairUtils.java:
##########
@@ -1348,11 +1349,22 @@ public static Collection<Range<Token>> split(Range<Token> tokenRange, int number
      * @return an estimate representing the number of partitions, size in range, and total size
      */
     static SizeEstimate getRangeSizeEstimate(RepairType repairType, String keyspace, String table, Range<Token> tokenRange)
+    {
+        return getRangeSizeEstimate(repairType, keyspace, table, tokenRange, getMemtableSize(keyspace, table));
+    }
+
+    private static long getMemtableSize(String keyspace, String table)

Review Comment:
   Running Alexe's review tools threw this finding:
   
   ```
   Potential bug: memtable bytes can be over-counted if the ring topology changes mid-repair-cycle
   
   RepairTokenRangeSplitter.getRepairAssignmentsForTable() falls back to recomputing a size estimate on-demand when it can't find a pre-cached one (RepairTokenRangeSplitter.java:489). That fallback (AutoRepairUtils.getRangeSizeEstimate, 4-arg overload) assigns the table's entire current memtable size to that single range, instead of a proportional share.
   
   This fallback isn't actually rare: the token ranges used to build the cache (PrioritizedRepairPlan.build, computed once up front) are re-fetched independently later for each keyspace (RepairTokenRangeSplitter.java:245), right before assignments are generated — and keyspaces are processed one at a time, so that re-fetch can happen much later than when the cache was built. If a node joins/leaves/moves in between, the ranges no longer match, the cache misses for every range of that table, and each range ends up claiming the full memtable size — inflating the byte total by up to Nx.
   
   That inflated number isn't just cosmetic — it feeds into maxBytesPerSchedule, so it can cause unrelated repair assignments later in the same schedule to be skipped.
   
   No test currently covers this fallback path.
   
   Suggested fix: pass the same token-range list used to build the KeyspaceRepairPlan through to assignment time instead of re-querying it, so there's no window for topology drift. If the fallback needs to stay, divide the recomputed memtable size across however many ranges are missing, the same way calcTotalBytesToBeRepaired already does.
   ```
   
   Which I think makes sense?



-- 
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