PHOENIX-7593: Enable CompactionScanner for flushes #2134

sanjeet006py · 2025-04-29T06:02:32Z

No description provided.

tkhurana · 2025-04-29T16:56:12Z

phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/CompactionScanner.java

+            // This will happen only during flushes as then we don't pass PTable object
+            // to determine emptyCF and emptyCQ
+            if (emptyCQ == EMPTY_BYTE_ARRAY) {
+                determineEmptyCfCq(result);


For not emptycfstore aren't we doing this check on every row

Yeah, will fix that. Thanks

kadirozde · 2025-04-29T17:02:22Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/TableTTLIT.java

-                    // Doing MAX_COLUMN_INDEX + 1 to account for empty cells
-                    assertEquals(TestUtil.getRawCellCount(conn, TableName.valueOf(tableName), row),
-                            rowUpdateCounter * (MAX_COLUMN_INDEX + 1));
+                    // At every flush, extra cell versions should be removed.


We need to rename this test method to testMinorCompactionAndFlushShouldNotRetainCellsWhenMaxLookbackIsDisabled

kadirozde · 2025-04-29T17:09:11Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/MaxLookbackExtendedIT.java

+            conn.commit();
+            injectEdge.incrementValue(MAX_LOOKBACK_AGE * 1000);
+            TestUtil.dumpTable(conn, dataTableName);
+            TestUtil.flush(utility, dataTableName);


Let's use TestUtil.getRawCellCount and verify that extra row versions are removed.

kadirozde · 2025-04-29T17:10:56Z

phoenix-core/src/it/java/org/apache/phoenix/end2end/MaxLookbackExtendedIT.java

+            TestUtil.dumpTable(conn, dataTableName);
+            ResultSet rs = stmt.executeQuery("select * from " + dataTableName + " where id = 'a'");
+            while(rs.next()) {
+                assertNotNull(rs.getString(3));


Let's verify the column values are equal to the expected values here.

virajjasani · 2025-04-29T17:32:12Z

phoenix-core-server/src/main/java/org/apache/phoenix/coprocessor/CompactionScanner.java

+        this.emptyCF = table != null ? SchemaUtil.getEmptyColumnFamily(table) : EMPTY_BYTE_ARRAY;
+        this.emptyCQ = table != null ? SchemaUtil.getEmptyColumnQualifier(table) : EMPTY_BYTE_ARRAY;


Why not keep emptyCF and emptyCQ as null if PTable is null, so that we can also incorporate this logic?

Instead of

if (ScanUtil.isEmptyColumn(cell, emptyCF, emptyCQ)) { index = addEmptyColumn(result, currentColumnCell, index, emptyColumn); } else { index = skipColumn(result, currentColumnCell, retainedCells, index); }

this

if (emptyCF != null && emptyCQ != null && ScanUtil.isEmptyColumn(cell, emptyCF, emptyCQ)) { index = addEmptyColumn(result, currentColumnCell, index, emptyColumn); } else { index = skipColumn(result, currentColumnCell, retainedCells, index); }

and similarly, if (emptyCQ == EMPTY_BYTE_ARRAY) too will be simple null check.

I don't think EMPTY_BYTE_ARRAY is allowed as CF:CQ, but while debugging, null check will be more readable rather than using incorrect values of emptyCF and emptyCQ for ScanUtil.isEmptyColumn?

By keeping emptyCF and emptyCQ values as null are we trying to optimize the if check? I actually kept it empty byte array to avoid null handling and nothing will match empty byte array.

I don't think EMPTY_BYTE_ARRAY is allowed as CF:CQ, but while debugging, null check will be more readable rather than using incorrect values of emptyCF and emptyCQ for ScanUtil.isEmptyColumn?

Got it, will change to storing null values. I agree this improves readability. Thanks

virajjasani · 2025-04-29T20:37:21Z

Not related to this PR, but as a general improvement, this method should not be named as isEmptyColumn() because it does not perform any empty column related check, all it checks for is whether the given cell has matching CF and CQ:

    public static boolean isEmptyColumn(Cell cell, byte[] emptyCF, byte[] emptyCQ) {
        return CellUtil.matchingFamily(cell, emptyCF, 0, emptyCF.length) &&
               CellUtil.matchingQualifier(cell, emptyCQ, 0, emptyCQ.length);
    }

We should remove the above utility because HBase CellUtil already provides exactly the same:

  public static boolean matchingColumn(final Cell left, final byte[] fam, final byte[] qual) {
    return matchingFamily(left, fam) && matchingQualifier(left, qual);
  }

(worth doing as separate Jira/PR though)

sanjeet006py · 2025-04-30T07:25:11Z

(worth doing as separate Jira/PR though)

Created JIRA: https://issues.apache.org/jira/browse/PHOENIX-7597

tkhurana · 2025-04-30T16:16:39Z

...re-server/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java

+    public InternalScanner preFlush(ObserverContext<RegionCoprocessorEnvironment> c, Store store,
+                                    InternalScanner scanner, FlushLifeCycleTracker tracker)
+            throws IOException {
+        if (!isPhoenixTableTTLEnabled(c.getEnvironment().getConfiguration())) {


We are using the same config to control both flushing and compaction. Will there be a scenario where we want to disable Phoenix compaction on flushing but still continue to use Phoenix compaction for major/minor compaction ?

Will there be a scenario where we want to disable Phoenix compaction on flushing but still continue to use Phoenix compaction for major/minor compaction ?

I think in general it will be good to have this flexibility. Shall I introduce a new config to enable disable preFlush hook separately?

Yeah I think it will be better because Phoenix compaction has already been running in production and this is a new feature so having this flexibility will be helpful. We can enable it by default.

This flexibility will be helpful. I think we can go ahead with merging the changes after config is added and perf can be done later. The config will anyways be helpful to turn off the feature, WDYT @tkhurana?

If we are fine with waiting for a day or 2 more then I can post the perf results. For single column family its done. I am currently, doing perf analysis of multi-CF. Thanks

@virajjasani perf analysis for multi-CF will take some time as I got to know that HBase flushTime metrics for multi-CF case sometimes could end up tracking combined time for flushing multiple CFs and other times only 1 CF. So, need to directly track time taken by StoreFlusher.
One idea is we wait for perf analysis before merging this PR but 5.3 release can go on or if we want this PR in 5.3 then for now we can disable preFlush hook via config and only enable it after perf analysis. I am bit inclined towards first approach. WDYT @virajjasani @tkhurana?

@sanjeet006py Instead of relying on metrics you could also use the log messages when flush happens. For example 2025-05-07 16:47:34,580 INFO [MemStoreFlusher.1] regionserver.HRegion - Finished flush of dataSize ~255.34 MB/267746710, heapSize ~256.01 MB/268445696, currentSize=10.04 MB/10529365 for 397f5412f43294d01081c54d7253d378 in 1986ms, sequenceid=207416387, compaction requested=false" Does that have the same problem too ?

Also, we don't need absolute numbers but just a comparison to make sure nothing is regressed.

Does that have the same problem too ?

Yes as this log line only tells us how much time all the stores which were to be flushed took to flush data as total. I added a log line in StoreFlusher and will test with that now.

Added the config to disable CompactionScanner for flushes.

tkhurana · 2025-04-30T16:25:06Z

@sanjeet006py Can you also do a perf study to rule out any performance degradation that can get introduced in the flushing path. We have some metrics at the regionserver like hbase.regionserver.FlushTime and at per table like hbase.regionserver.Namespace_default_table_<TABLENAME>_metric_flushTime_95th_percentile

sanjeet006py · 2025-05-09T06:50:48Z

@tkhurana @virajjasani the perf analysis is done: https://docs.google.com/document/d/1oQzEMP4LXOFxLHlKt1SZ5uvRLd3Vk90x39gn1hVBn0Y/edit?tab=t.0#heading=h.32xuccojgowv. Overall I see enabling CompactionScanner for flushes will have some overhead (as expected) but no big enough to cause performance degradation. Thanks

PHOENIX-7593: Enable CompactionScanner for flushes

b42a428

sanjeet006py requested review from kadirozde, tkhurana and virajjasani April 29, 2025 14:33

tkhurana reviewed Apr 29, 2025

View reviewed changes

kadirozde reviewed Apr 29, 2025

View reviewed changes

virajjasani reviewed Apr 29, 2025

View reviewed changes

Address PR comments

2bfcc8e

sanjeet006py requested review from tkhurana, kadirozde and virajjasani April 30, 2025 07:36

Address style check issues

cac4260

tkhurana reviewed Apr 30, 2025

View reviewed changes

Sanjeet Malhotra added 2 commits May 9, 2025 13:05

Add flag to disable compaction scanner for flushes

924e615

Fix style checks

f1c32f0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHOENIX-7593: Enable CompactionScanner for flushes #2134

PHOENIX-7593: Enable CompactionScanner for flushes #2134

sanjeet006py commented Apr 29, 2025

tkhurana Apr 29, 2025

sanjeet006py Apr 29, 2025

kadirozde Apr 29, 2025

kadirozde Apr 29, 2025

kadirozde Apr 29, 2025

virajjasani Apr 29, 2025 •

edited

Loading

sanjeet006py Apr 29, 2025

sanjeet006py Apr 29, 2025

virajjasani commented Apr 29, 2025 •

edited

Loading

sanjeet006py commented Apr 30, 2025 •

edited

Loading

tkhurana Apr 30, 2025

sanjeet006py Apr 30, 2025

tkhurana Apr 30, 2025

virajjasani May 6, 2025

sanjeet006py May 6, 2025

sanjeet006py May 7, 2025 •

edited

Loading

tkhurana May 7, 2025 •

edited

Loading

tkhurana May 7, 2025

sanjeet006py May 7, 2025

sanjeet006py May 9, 2025

tkhurana commented Apr 30, 2025 •

edited

Loading

sanjeet006py commented May 9, 2025 •

edited

Loading

		this.emptyCF = table != null ? SchemaUtil.getEmptyColumnFamily(table) : EMPTY_BYTE_ARRAY;
		this.emptyCQ = table != null ? SchemaUtil.getEmptyColumnQualifier(table) : EMPTY_BYTE_ARRAY;

PHOENIX-7593: Enable CompactionScanner for flushes #2134

Are you sure you want to change the base?

PHOENIX-7593: Enable CompactionScanner for flushes #2134

Conversation

sanjeet006py commented Apr 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajjasani Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virajjasani commented Apr 29, 2025 • edited Loading

sanjeet006py commented Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjeet006py May 7, 2025 • edited Loading

Choose a reason for hiding this comment

tkhurana May 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkhurana commented Apr 30, 2025 • edited Loading

sanjeet006py commented May 9, 2025 • edited Loading

virajjasani Apr 29, 2025 •

edited

Loading

virajjasani commented Apr 29, 2025 •

edited

Loading

sanjeet006py commented Apr 30, 2025 •

edited

Loading

sanjeet006py May 7, 2025 •

edited

Loading

tkhurana May 7, 2025 •

edited

Loading

tkhurana commented Apr 30, 2025 •

edited

Loading

sanjeet006py commented May 9, 2025 •

edited

Loading