github-actions[bot] commented on code in PR #61915:
URL: https://github.com/apache/doris/pull/61915#discussion_r3013274419
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -560,6 +561,12 @@ public void addActiveTablets(List<Long> tabletIds) {
if (Config.cloud_get_tablet_stats_version == 1 || tabletIds == null ||
tabletIds.isEmpty()) {
return;
}
+ String ignoreTablets = DebugPointUtil.getDebugParamOrDefault(
+ "FE.CloudTabletStatMgr.addActiveTablets.ignore.tablets", "");
+ if (!ignoreTablets.isEmpty() && tabletIds.stream().anyMatch(id ->
ignoreTablets.contains(String.valueOf(id)))) {
+ LOG.info("ignore adding active tablets: {}, debug param: {}",
tabletIds, ignoreTablets);
Review Comment:
**Bug: Substring false-positive in tablet ID matching**
`ignoreTablets.contains(String.valueOf(id))` performs substring matching on
a comma-separated string. If `ignoreTablets = "12345"`, then tablet IDs `123`,
`1234`, `2345`, `234`, `345`, `45`, etc. would all falsely match.
Other debug points in the codebase (e.g., `Backend.isQueryAvailable` in
`Backend.java:545`, `HeartbeatMgr.java:329`) handle this correctly by splitting
on comma first:
```java
Arrays.stream(ignoreTablets.split(",")).anyMatch(s ->
s.equals(String.valueOf(id)))
```
Please use the split-then-exact-match pattern to avoid false positives.
##########
regression-test/suites/cloud_p0/tablets/test_tablet_stat_syncer.groovy:
##########
@@ -0,0 +1,149 @@
+// 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.
+
+import org.apache.doris.regression.suite.ClusterOptions
+
+suite("test_tablet_stat_syncer", "docker") {
+ if (!isCloudMode()) {
+ return
+ }
+
+ def options = new ClusterOptions()
+ options.setFeNum(1)
+ options.setBeNum(1)
+ options.cloudMode = true
+ options.enableDebugPoints()
+
+ docker(options) {
+ def tbl = 'test_tablet_stat_syncer_tbl'
+ sql """ DROP TABLE IF EXISTS ${tbl} """
+
+ def configResult = sql_return_maparray """ ADMIN SHOW FRONTEND CONFIG
LIKE '%cloud_get_tablet_stats_version%' """
+ assertTrue(configResult.size() > 0, "Expected to find
cloud_get_tablet_stats_version config")
+ def tabletStatsVersion = configResult[0].Value.toInteger()
+ logger.info("cloud_get_tablet_stats_version = ${tabletStatsVersion}")
+ if (tabletStatsVersion != 2) {
+ logger.info("cloud_get_tablet_stats_version is not 2, skip test")
+ return
+ }
+
+ def intervalConfigResult = sql_return_maparray """ ADMIN SHOW FRONTEND
CONFIG LIKE '%tablet_stat_update_interval_second%' """
+ assertTrue(intervalConfigResult.size() > 0, "Expected to find
tablet_stat_update_interval_second config")
+ def statUpdateInterval = intervalConfigResult[0].Value.toInteger()
+ logger.info("tablet_stat_update_interval_second =
${statUpdateInterval}")
+
+ sql """
+ CREATE TABLE IF NOT EXISTS ${tbl} (
+ id INT NOT NULL,
+ name VARCHAR(50) NOT NULL,
+ value INT NOT NULL
+ )
+ ENGINE = olap
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1"
+ );
+ """
+
+ def result = sql_return_maparray """ select * from ${tbl} """
+ assertEquals(0, result.size())
+
+ sql """ INSERT INTO ${tbl} VALUES (1, 'initial', 100) """
+ result = sql_return_maparray """ select * from ${tbl} where id = 1 """
+ assertEquals(1, result.size())
+
+ def tablets = sql_return_maparray """ show tablets from ${tbl} """
+ assertTrue(tablets.size() > 0, "Expected at least one tablet")
+ def tabletIds = tablets.collect { it.TabletId }.join(",")
+ logger.info("Tablet IDs: ${tabletIds}")
+
+ def initialRowCount = tablets[0].RowCount
+ logger.info("Initial RowCount: ${initialRowCount}")
+
+
GetDebugPoint().enableDebugPointForAllFEs("FE.CloudTabletStatMgr.addActiveTablets.ignore.tablets",
[value: tabletIds])
+
+ sql """ INSERT INTO ${tbl} VALUES (2, 'test', 200) """
+ result = sql_return_maparray """ select * from ${tbl} where id = 2 """
+ assertEquals(1, result.size(), "Data should be visible in query")
+
+ def tabletsAfterInsert = sql_return_maparray """ show tablets from
${tbl} """
+ def rowCountAfterInsert = tabletsAfterInsert[0].RowCount
+ logger.info("RowCount after insert (before sync):
${rowCountAfterInsert}")
+
+
GetDebugPoint().disableDebugPointForAllFEs("FE.CloudTabletStatMgr.addActiveTablets.ignore.tablets")
+
+ def maxWaitMs = (statUpdateInterval * 3 + 5) * 1000
+ def intervalMs = 1000
+ def startTime = System.currentTimeMillis()
+ def updated = false
+ while (true) {
+ def tabletsNow = sql_return_maparray """ show tablets from ${tbl}
"""
+ def rowCountNow = tabletsNow[0].RowCount.toInteger()
+ logger.info("RowCount get: ${rowCountNow}")
+ if (rowCountNow == 2) {
+ logger.info("RowCount updated: ${rowCountNow}")
+ updated = true
+ break
+ }
+ if (System.currentTimeMillis() - startTime > maxWaitMs) {
+ throw new IllegalStateException("Timeout waiting for tablet
stat update. Waited ${maxWaitMs}ms, RowCount still ${rowCountNow}")
+ }
+ Thread.sleep(intervalMs)
+ }
+ assertTrue(updated, "Tablet stat should eventually update after debug
point disabled")
+ tabletsAfterInsert = sql_return_maparray """ show tablets from ${tbl}
"""
+ rowCountAfterInsert = tabletsAfterInsert[0].RowCount.toInteger()
+ logger.info("RowCount after stat sync: ${rowCountAfterInsert}")
+ assertEquals(2, rowCountAfterInsert, "RowCount should reflect the new
row after stat sync")
+
+ sql """ INSERT INTO ${tbl} VALUES (3, 'force_sync', 300) """
+ def tabletsBeforeSync = sql_return_maparray """ show tablets from
${tbl} """
+ def localDataSizeBeforeSync = tabletsBeforeSync[0].LocalDataSize
+ def rowCountBeforeSync = tabletsBeforeSync[0].RowCount
+ logger.info("LocalDataSize before force sync:
${localDataSizeBeforeSync}, RowCount: ${rowCountBeforeSync}")
+
+ sql """ set cloud_force_sync_tablet_stats = true """
+ def tabletsAfterForceSync = sql_return_maparray """ show tablets from
${tbl} """
+ def localDataSizeAfterForceSync =
tabletsAfterForceSync[0].LocalDataSize
+ def rowCountAfterForceSync = tabletsAfterForceSync[0].RowCount
+ logger.info("LocalDataSize after force sync:
${localDataSizeAfterForceSync}, RowCount: ${rowCountAfterForceSync}")
Review Comment:
**Test weakness: force-sync section doesn't assert the immediate effect**
After setting `cloud_force_sync_tablet_stats = true` and reading tablet
stats (lines 120-124), the test only logs the results but never asserts that
`rowCountAfterForceSync == 3`. It then falls through to a polling loop that
waits for eventual consistency — which is the same verification pattern as the
non-force-sync path above.
To actually verify that force sync works (i.e., provides an *immediate*
update), consider adding:
```groovy
assertEquals(3, rowCountAfterForceSync.toInteger(), "RowCount should be
updated immediately after force sync")
```
Otherwise this section tests nothing beyond what the first section already
tests.
##########
regression-test/suites/cloud_p0/version/test_version_syncer.groovy:
##########
@@ -0,0 +1,105 @@
+// 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.
+
+suite("test_version_syncer", "nonConcurrent") {
+ if (!isCloudMode()) {
+ return
+ }
+
+ def tbl = 'test_version_syncer_tbl'
+ sql """ DROP TABLE IF EXISTS ${tbl} """
+
+ def enableSyncerConfig = sql_return_maparray """ ADMIN SHOW FRONTEND
CONFIG LIKE '%cloud_enable_version_syncer%' """
+ assertTrue(enableSyncerConfig.size() > 0, "Expected to find
cloud_enable_version_syncer config")
+ def enableSyncer = enableSyncerConfig[0].Value.toBoolean()
+ logger.info("cloud_enable_version_syncer = ${enableSyncer}")
+ if (!enableSyncer) {
+ logger.info("FE.CloudGlobalTransactionMgr.updateVersion.disabled")
+ return
+ }
+ def configResult = sql_return_maparray """ ADMIN SHOW FRONTEND CONFIG LIKE
'%cloud_version_syncer_interval_second%' """
+ assertTrue(configResult.size() > 0, "Expected to find
cloud_version_syncer_interval_second config")
+ def syncerInterval = configResult[0].Value.toInteger()
+ logger.info("cloud_version_syncer_interval_second = ${syncerInterval}")
+
+ onFinish {
+ GetDebugPoint().clearDebugPointsForAllFEs()
+ }
+
+ sql """
+ CREATE TABLE IF NOT EXISTS ${tbl} (
+ id INT NOT NULL,
+ name VARCHAR(50) NOT NULL,
+ value INT NOT NULL
+ )
+ DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 1
+ PROPERTIES (
+ "replication_num" = "1"
+ );
+ """
+
+ def result = sql_return_maparray """ select * from ${tbl} """
+ assertEquals(0, result.size())
+
+ sql """ INSERT INTO ${tbl} VALUES (1, 'initial', 100) """
+ result = sql_return_maparray """ select * from ${tbl} where id = 1 """
+ assertEquals(1, result.size())
+
+
DebugPoint.enableDebugPointForAllFEs('FE.CloudGlobalTransactionMgr.updateVersion.disabled')
+
+ sql """ INSERT INTO ${tbl} VALUES (2, 'test', 200) """
+ result = sql_return_maparray """ select * from ${tbl} where id = 2 """
+ assertEquals(0, result.size(), "Data should not be visible before version
sync")
+
+ def maxWaitMs = (syncerInterval * 2 + 5) * 1000
+ def intervalMs = 1000
+ def startTime = System.currentTimeMillis()
+ def found = false
+ while (true) {
+ result = sql_return_maparray """ select * from ${tbl} where id = 2 """
+ if (result.size() == 1) {
+ assertEquals('test', result[0].name)
+ assertEquals(200, result[0].value)
+ found = true
+ break
+ }
+ if (System.currentTimeMillis() - startTime > maxWaitMs) {
+ throw new IllegalStateException("Timeout waiting for data to be
visible. Waited ${maxWaitMs}ms")
+ }
+ Thread.sleep(intervalMs)
+ }
+ assertTrue(found, "Data should eventually be visible after version syncer
runs")
+
+ result = sql_return_maparray """ select * from ${tbl} order by id """
+ assertEquals(2, result.size())
+
+ // Test cloud_force_sync_version session variable
+ // Step 1: Insert data with version sync disabled, query should return no
data
+ sql """ INSERT INTO ${tbl} VALUES (3, 'force_sync', 300) """
+ result = sql_return_maparray """ select * from ${tbl} where id = 3 """
+ assertEquals(0, result.size(), "Data should not be visible before force
sync")
+
+ // Step 2: Enable session variable cloud_force_sync_version = true and
execute show partitions
+ sql """ set cloud_force_sync_version = true """
+ def showPartitionsResult = sql_return_maparray """ show partitions from
${tbl} """
Review Comment:
**Minor: `cloud_force_sync_version` is never reset to `false`**
Line 98 sets `cloud_force_sync_version = true` but it is never set back to
`false`. The tablet stat syncer test (line 125) properly resets its equivalent
variable. While session-scoped variables are cleaned up when the session ends,
it's good practice to reset it for consistency and to avoid affecting
subsequent queries in the same session.
--
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]
