Copilot commented on code in PR #61915:
URL: https://github.com/apache/doris/pull/61915#discussion_r3013204787
##########
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:
`ignoreTablets.contains(String.valueOf(id))` can match substrings (e.g.,
id=12 matches "112,113"), causing unrelated tablets to be ignored. Parse the
debug param as a delimiter-separated set (e.g., split by comma and compare as
longs/strings with exact equality) to avoid false positives.
##########
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} """
+ logger.info("Show partitions result: ${showPartitionsResult}")
+
+ // Step 3: Query again, data should now be visible
+ result = sql_return_maparray """ select * from ${tbl} where id = 3 """
+ assertEquals(1, result.size(), "Data should be visible after force sync")
Review Comment:
`set cloud_force_sync_version = true` is not reset to its default value. To
avoid leaking session state into later statements/tests executed on the same
connection, reset it (e.g., set to false) after the force-sync check (ideally
in `onFinish`).
```suggestion
try {
def showPartitionsResult = sql_return_maparray """ show partitions
from ${tbl} """
logger.info("Show partitions result: ${showPartitionsResult}")
// Step 3: Query again, data should now be visible
result = sql_return_maparray """ select * from ${tbl} where id = 3
"""
assertEquals(1, result.size(), "Data should be visible after force
sync")
} finally {
// Reset session variable to avoid leaking state into other
statements/tests
sql """ set cloud_force_sync_version = false """
}
```
##########
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")
Review Comment:
The debug point is intended to keep tablet stats stale, but the test doesn’t
assert that `RowCount` stayed unchanged while it was enabled (and
`initialRowCount` becomes unused). Add an assertion on `rowCountAfterInsert`
(e.g., equals `initialRowCount`/1) before disabling the debug point so the test
can’t pass if stats update immediately.
##########
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
+ }
Review Comment:
This test relies on FE debug points, but it doesn’t check that
`enable_debug_points` is enabled on the target cluster. Consider skipping the
test (or running in a docker cluster with `options.enableDebugPoints()`) when
`getFeConfig('enable_debug_points') != 'true'`, otherwise the debug point may
have no effect and assertions can fail.
```suggestion
}
def enableDebugPoints = getFeConfig("enable_debug_points")
if (enableDebugPoints != "true") {
logger.info("enable_debug_points is not enabled, skip
test_version_syncer")
return
}
```
--
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]
