Copilot commented on code in PR #1651:
URL: https://github.com/apache/fluss/pull/1651#discussion_r2328957856
##########
fluss-lake/fluss-lake-iceberg/src/test/java/org/apache/fluss/lake/iceberg/testutils/FlinkIcebergTieringTestBase.java:
##########
@@ -185,18 +188,25 @@ protected static Catalog getIcebergCatalog() {
}
protected long createPkTable(TablePath tablePath) throws Exception {
- return createPkTable(tablePath, 1);
+ return createPkTable(tablePath, false);
Review Comment:
[nitpick] The method signatures are inconsistent. `createLogTable` with
auto-compaction calls the 4-parameter version with hardcoded `false` for
`isPartitioned`, but `createPkTable` doesn't have an equivalent `isPartitioned`
parameter. Consider making the API consistent by either adding partition
support to PK tables or removing the hardcoded parameter.
##########
fluss-lake/fluss-lake-iceberg/src/test/java/org/apache/fluss/lake/iceberg/testutils/FlinkIcebergTieringTestBase.java:
##########
@@ -185,18 +188,25 @@ protected static Catalog getIcebergCatalog() {
}
protected long createPkTable(TablePath tablePath) throws Exception {
- return createPkTable(tablePath, 1);
+ return createPkTable(tablePath, false);
+ }
+
+ protected long createPkTable(TablePath tablePath, boolean
enableAutoCompaction)
+ throws Exception {
+ return createPkTable(tablePath, 1, enableAutoCompaction);
}
protected long createLogTable(TablePath tablePath) throws Exception {
- return createLogTable(tablePath, 1);
+ return createLogTable(tablePath, false);
}
- protected long createLogTable(TablePath tablePath, int bucketNum) throws
Exception {
- return createLogTable(tablePath, bucketNum, false);
+ protected long createLogTable(TablePath tablePath, boolean
enableAutoCompaction)
+ throws Exception {
+ return createLogTable(tablePath, 1, false, enableAutoCompaction);
Review Comment:
[nitpick] The method signatures are inconsistent. `createLogTable` with
auto-compaction calls the 4-parameter version with hardcoded `false` for
`isPartitioned`, but `createPkTable` doesn't have an equivalent `isPartitioned`
parameter. Consider making the API consistent by either adding partition
support to PK tables or removing the hardcoded parameter.
##########
fluss-lake/fluss-lake-iceberg/src/test/java/org/apache/fluss/lake/iceberg/testutils/FlinkIcebergTieringTestBase.java:
##########
@@ -185,18 +188,25 @@ protected static Catalog getIcebergCatalog() {
}
protected long createPkTable(TablePath tablePath) throws Exception {
- return createPkTable(tablePath, 1);
+ return createPkTable(tablePath, false);
+ }
+
+ protected long createPkTable(TablePath tablePath, boolean
enableAutoCompaction)
+ throws Exception {
+ return createPkTable(tablePath, 1, enableAutoCompaction);
}
protected long createLogTable(TablePath tablePath) throws Exception {
- return createLogTable(tablePath, 1);
+ return createLogTable(tablePath, false);
Review Comment:
[nitpick] The method signatures are inconsistent. `createLogTable` with
auto-compaction calls the 4-parameter version with hardcoded `false` for
`isPartitioned`, but `createPkTable` doesn't have an equivalent `isPartitioned`
parameter. Consider making the API consistent by either adding partition
support to PK tables or removing the hardcoded parameter.
##########
fluss-lake/fluss-lake-iceberg/src/main/java/org/apache/fluss/lake/iceberg/tiering/IcebergLakeWriter.java:
##########
@@ -137,9 +129,11 @@ public void close() throws IOException {
compactionFuture.cancel(true);
}
- if (compactionExecutor != null
- && !compactionExecutor.awaitTermination(30,
TimeUnit.SECONDS)) {
- LOG.warn("Fail to close compactionExecutor.");
+ if (compactionExecutor != null) {
+ compactionExecutor.shutdown();
+ if (!compactionExecutor.awaitTermination(30,
TimeUnit.SECONDS)) {
+ LOG.warn("Fail to close compactionExecutor.");
+ }
}
Review Comment:
[nitpick] The timeout value of 30 seconds is a magic number. Consider
extracting this to a named constant or configuration parameter to improve
maintainability.
##########
fluss-lake/fluss-lake-iceberg/src/test/java/org/apache/fluss/lake/iceberg/testutils/FlinkIcebergTieringTestBase.java:
##########
@@ -185,18 +188,25 @@ protected static Catalog getIcebergCatalog() {
}
protected long createPkTable(TablePath tablePath) throws Exception {
- return createPkTable(tablePath, 1);
+ return createPkTable(tablePath, false);
+ }
+
+ protected long createPkTable(TablePath tablePath, boolean
enableAutoCompaction)
+ throws Exception {
+ return createPkTable(tablePath, 1, enableAutoCompaction);
}
Review Comment:
[nitpick] The method signatures are inconsistent. `createLogTable` with
auto-compaction calls the 4-parameter version with hardcoded `false` for
`isPartitioned`, but `createPkTable` doesn't have an equivalent `isPartitioned`
parameter. Consider making the API consistent by either adding partition
support to PK tables or removing the hardcoded parameter.
```suggestion
return createPkTable(tablePath, false, false);
}
protected long createPkTable(TablePath tablePath, boolean
enableAutoCompaction)
throws Exception {
return createPkTable(tablePath, false, enableAutoCompaction);
}
protected long createPkTable(TablePath tablePath, boolean isPartitioned,
boolean enableAutoCompaction)
throws Exception {
if (isPartitioned) {
throw new UnsupportedOperationException("Partitioned PK tables
are not supported.");
}
// PK table creation logic (preserving current behavior)
Schema.Builder schemaBuilder =
Schema.newBuilder().column("a", DataTypes.INT()).column("b",
DataTypes.STRING());
TableDescriptor.Builder tableBuilder =
TableDescriptor.builder()
.primaryKey("a")
.distributedBy(1, "a")
.property(ConfigOptions.TABLE_DATALAKE_ENABLED.key(), "true")
.property(ConfigOptions.TABLE_DATALAKE_FRESHNESS,
Duration.ofMillis(500));
if (enableAutoCompaction) {
tableBuilder.property(ConfigOptions.TABLE_DATALAKE_AUTO_COMPACTION.key(),
"true");
}
tableBuilder.schema(schemaBuilder.build());
return createTable(tablePath, tableBuilder.build());
}
```
--
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]
