Copilot commented on code in PR #61887:
URL: https://github.com/apache/doris/pull/61887#discussion_r3008495579
##########
be/src/cloud/cloud_tablet.cpp:
##########
@@ -469,11 +469,18 @@ void
CloudTablet::add_rowsets(std::vector<RowsetSharedPtr> to_add, bool version_
}
// clang-format off
auto self =
std::dynamic_pointer_cast<CloudTablet>(shared_from_this());
+ auto file_system = rowset_meta->fs();
+ if (!file_system) {
+ LOG(WARNING) << "failed to get file system for
tablet_id="
+ << _tablet_meta->tablet_id() << ",
rowset_id="
+ << rowset_meta->rowset_id();
Review Comment:
`rowset_meta->fs()` is computed inside the per-segment loop, so it will be
called once per segment even though the filesystem is identical for all
segments of the same rowset. Consider computing the filesystem once per rowset
(outside the `for (int seg_id...)` loop) and reusing it to reduce repeated work
and keep the loop body simpler.
##########
regression-test/suites/cloud_p0/balance/test_balance_warm_up_with_compaction_use_peer_cache.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_warm_up_with_compaction_use_peer_cache',
'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from environment for deterministic
behavior
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(from ENABLE_PACKED_FILE=${enablePackedFileEnv})")
```
##########
regression-test/suites/cloud_p0/tablets/test_clean_tablet_when_drop_force_table.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_clean_tablet_when_drop_force_table', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Control packed_file via environment variable for deterministic
behavior.
// Set ENABLE_PACKED_FILE to "true" or "false" (defaults to false if
unset).
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = (enablePackedFileEnv != null) ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile},
source=env(ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/balance/test_peer_read_async_warmup.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_peer_read_async_warmup', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from environment for deterministic
behavior
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = (enablePackedFileEnv != null) ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}" +
(enablePackedFileEnv != null ? " (from ENABLE_PACKED_FILE)" : "
(default)"))
```
##########
regression-test/suites/cloud_p0/balance/test_balance_metrics.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_metrics', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable input (environment
variable) to keep tests deterministic
def enablePackedFileEnv = System.getenv('ENABLE_PACKED_FILE')
def enablePackedFile = (enablePackedFileEnv != null) ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(from env ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/balance/test_alter_compute_group_properties.groovy:
##########
@@ -22,12 +22,20 @@ suite('test_alter_compute_group_properties', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
##########
regression-test/suites/cloud_p0/balance/test_balance_warm_up_use_peer_cache.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_warm_up_use_peer_cache', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from environment to keep tests
deterministic.
// CI can run this suite with ENABLE_PACKED_FILE=true/false to cover
both scenarios.
def enablePackedFileEnv = System.getenv('ENABLE_PACKED_FILE')
def enablePackedFile = enablePackedFileEnv != null ?
enablePackedFileEnv.toBoolean() : false
```
##########
regression-test/suites/cloud_p0/tablets/test_clean_tablet_when_rebalance.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_clean_tablet_when_rebalance', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable input (env var) to keep
test deterministic
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv == null ? false :
Boolean.parseBoolean(enablePackedFileEnv)
logger.info("Running test with enable_packed_file=${enablePackedFile}
(source env ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/balance/test_balance_use_compute_group_properties.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_use_compute_group_properties', 'docker')
{
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable source (env var) to keep
the test deterministic.
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(from ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/balance/test_balance_warm_up_sync_global_config.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_warm_up_sync_cache', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable input (environment
variable) to keep the test deterministic.
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/balance/test_expanding_node_balance.groovy:
##########
@@ -23,6 +23,10 @@ suite('test_expanding_node_balance', 'docker') {
return;
}
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable input (env var) for
reproducible tests
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(source=${enablePackedFileEnv != null ? 'env' : 'default'})")
```
##########
regression-test/suites/cloud_p0/tablets/test_clean_stale_rs_index_file_cache.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_clean_stale_rs_index_file_cache', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Deterministically enable or disable packed_file based on environment
variable
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
enablePackedFileEnv.toBoolean() : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/cache/test_load_cache.groovy:
##########
@@ -33,6 +33,10 @@ changes to statistics are possible, only a reasonable range
is required.
*/
suite('test_load_cache', 'docker') {
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Configure enable_packed_file via environment variable for
deterministic behavior.
// Set ENABLE_PACKED_FILE=true or ENABLE_PACKED_FILE=false to control
this value; default is false if unset.
def enablePackedFile = System.getenv('ENABLE_PACKED_FILE')?.toBoolean()
?: false
```
##########
regression-test/suites/cloud_p0/balance/test_warmup_rebalance.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_warmup_rebalance_in_cloud', 'multi_cluster,
docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
##########
regression-test/suites/cloud_p0/cache/test_topn_broadcast.groovy:
##########
@@ -21,6 +21,9 @@ import org.apache.doris.regression.suite.ClusterOptions
suite("test_topn_broadcast", "docker") {
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
##########
regression-test/suites/cloud_p0/tablets/test_clean_stale_rs_file_cache.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_clean_stale_rs_file_cache', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from environment for deterministic
behavior
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
Boolean.parseBoolean(enablePackedFileEnv) : true
logger.info("Running test with enable_packed_file=${enablePackedFile}" +
(enablePackedFileEnv != null ? " (from ENABLE_PACKED_FILE env)"
: " (default)"))
```
##########
regression-test/suites/cloud_p0/balance/test_balance_warm_up_task_abnormal.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_warm_up_task_abnormal', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
+ logger.info("Running test with enable_packed_file=${enablePackedFile}")
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable input (env var) instead of
randomness
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = (enablePackedFileEnv != null) ?
Boolean.parseBoolean(enablePackedFileEnv) : false
logger.info("Running test with enable_packed_file=${enablePackedFile}
(ENABLE_PACKED_FILE='${enablePackedFileEnv}')")
```
##########
regression-test/suites/cloud_p0/balance/test_balance_warm_up.groovy:
##########
@@ -22,6 +22,11 @@ suite('test_balance_warm_up', 'docker') {
if (!isCloudMode()) {
return;
}
+
+ // Randomly enable or disable packed_file to test both scenarios
+ def enablePackedFile = new Random().nextBoolean()
Review Comment:
This test is now non-deterministic because `enable_packed_file` is chosen
via `new Random().nextBoolean()`. That makes failures harder to reproduce and
also means CI only exercises one scenario per run (50% chance). Prefer running
both values explicitly (e.g., loop over [true,false] / two docker runs) or
driving the value from a stable input (suite param/env var) with logging of the
chosen value.
```suggestion
// Determine enable_packed_file from a stable input (env var) to avoid
nondeterminism
def enablePackedFileEnv = System.getenv("ENABLE_PACKED_FILE")
def enablePackedFile = enablePackedFileEnv != null ?
Boolean.parseBoolean(enablePackedFileEnv) : false
```
--
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]
