You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "slfan1989 (via GitHub)" <gi...@apache.org> on 2025/08/16 10:09:42 UTC
[PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
slfan1989 opened a new pull request, #13832:
URL: https://github.com/apache/iceberg/pull/13832
This pull request improves the readability and maintainability of the following configuration classes by enhancing the descriptions for ConfigOptions and adding necessary comments:
- FlinkMaintenanceConfig:
Added clear descriptions for configuration options such as LOCK_CHECK_DELAY_OPTION, PARALLELISM_OPTION, RATE_LIMIT_OPTION, and SLOT_SHARING_GROUP_OPTION.
Added comments explaining the purpose and function of each configuration option.
- LockConfig:
Added detailed descriptions for configuration options like LOCK_TYPE_OPTION, LOCK_ID_OPTION, JdbcLockConfig, and ZkLockConfig related to different locking mechanisms.
Improved code clarity by explaining the role of each configuration option for locking (e.g., JDBC_URI_OPTION, ZK_URI_OPTION, etc.).
- RewriteDataFilesConfig:
Enhanced the descriptions of configuration options, including MAX_BYTES_OPTION, PARTIAL_PROGRESS_MAX_COMMITS_OPTION, SCHEDULE_ON_COMMIT_COUNT_OPTION, SCHEDULE_ON_DATA_FILE_COUNT_OPTION, and more.
Added helpful comments to clarify the purpose of each configuration setting related to data rewriting and its usage in Flink-based maintenance tasks.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu merged PR #13832:
URL: https://github.com/apache/iceberg/pull/13832
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3219049087
@stevenzwu @pvary Could you kindly review this PR again? Thank you very much! The previous suggestions have been addressed and modified accordingly.
cc: @mxm @Guosmilesmile
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3235014549
thanks @slfan1989 for the contribution and everyone for the review
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2281302546
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
public static final String LOCK_CHECK_DELAY = PREFIX + "lock-check-delay-seconds";
public static final ConfigOption<Long> LOCK_CHECK_DELAY_OPTION =
ConfigOptions.key(LOCK_CHECK_DELAY)
.longType()
- .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT);
+ .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT)
+ .withDescription(
+ "The delay time (in seconds) between each lock check during the rewrite operation.");
+ // Configuration for parallelism
+ // This setting controls the parallelism level for the maintenance tasks,
+ // determining how many tasks can run concurrently
public static final String PARALLELISM = PREFIX + "parallelism";
public static final ConfigOption<Integer> PARALLELISM_OPTION =
- ConfigOptions.key(PARALLELISM).intType().defaultValue(ExecutionConfig.PARALLELISM_DEFAULT);
+ ConfigOptions.key(PARALLELISM)
+ .intType()
+ .defaultValue(ExecutionConfig.PARALLELISM_DEFAULT)
+ .withDescription(
+ "The parallelism level for the maintenance task. "
+ + "Determines how many tasks can run concurrently.");
Review Comment:
seems that the second part is redundant. or maybe merge the two as `The number of parallel tasks for the maintenance action`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3238946433
> > @slfan1989: Thanks for the contribution. Please don't forget to backport the changes to the other Flink branches
>
> @pvary Thank you for the reminder! The backport has been completed in #13944.
Thanks @slfan1989! I have missed that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "mxm (via GitHub)" <gi...@apache.org>.
mxm commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2281574664
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -75,6 +103,11 @@ public long rateLimit() {
.parse();
}
+ /**
+ * Gets the parallelism value for maintenance tasks.
+ *
+ * @return The parallelism for maintenance tasks
+ */
Review Comment:
Same here, I would remove the `@return` line.
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
public static final String LOCK_CHECK_DELAY = PREFIX + "lock-check-delay-seconds";
public static final ConfigOption<Long> LOCK_CHECK_DELAY_OPTION =
ConfigOptions.key(LOCK_CHECK_DELAY)
.longType()
- .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT);
+ .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT)
+ .withDescription(
+ "The delay time (in seconds) between each lock check during the rewrite operation.");
+ // Configuration for parallelism
+ // This setting controls the parallelism level for the maintenance tasks,
+ // determining how many tasks can run concurrently
public static final String PARALLELISM = PREFIX + "parallelism";
public static final ConfigOption<Integer> PARALLELISM_OPTION =
- ConfigOptions.key(PARALLELISM).intType().defaultValue(ExecutionConfig.PARALLELISM_DEFAULT);
+ ConfigOptions.key(PARALLELISM)
+ .intType()
+ .defaultValue(ExecutionConfig.PARALLELISM_DEFAULT)
+ .withDescription(
+ "The parallelism level for the maintenance task. "
+ + "Determines how many tasks can run concurrently.");
+ // Configuration for rate limit (in seconds)
+ // This setting controls the rate at which maintenance operations can occur,
+ // limiting the number of operations per second
Review Comment:
Same here, do we need this comment with the description below?
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -66,6 +89,11 @@ public FlinkMaintenanceConfig(
this.confParser = new FlinkConfParser(table, writeOptions, readableConfig);
}
+ /**
+ * Gets the rate limit value (in seconds) for maintenance operations.
+ *
+ * @return The rate limit for maintenance operations
+ */
Review Comment:
NIT: I find the `@return` a bit too verbose.
```suggestion
/**
* Gets the rate limit value (in seconds) for maintenance operations.
*/
```
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
Review Comment:
With the description below, should we remove this comment?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3235338181
> thanks @slfan1989 for the contribution and everyone for the review
Many thanks to everyone for your help in reviewing the code!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2281411573
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/RewriteDataFilesConfig.java:
##########
@@ -31,39 +31,73 @@
public class RewriteDataFilesConfig {
public static final String PREFIX = FlinkMaintenanceConfig.PREFIX + "rewrite.";
+ // Configuration option for maximum rewrite bytes
public static final String MAX_BYTES = PREFIX + "max-bytes";
public static final ConfigOption<Long> MAX_BYTES_OPTION =
- ConfigOptions.key(MAX_BYTES).longType().defaultValue(Long.MAX_VALUE);
+ ConfigOptions.key(MAX_BYTES)
+ .longType()
+ .defaultValue(Long.MAX_VALUE)
+ .withDescription(
+ "The maximum number of bytes allowed for the rewrite operation. "
+ + "If the total size of the data files to be rewritten exceeds this value, "
+ + "the rewrite operation will be split into multiple parts.");
Review Comment:
Instead of using "multiple parts," which can be ambiguous, it would be clearer to say "multiple scheduling tasks."
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3238402539
> @slfan1989: Thanks for the contribution. Please don't forget to backport the changes to the other Flink branches
@pvary Thank you for the reminder! The backport has been completed in #13944.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2295056399
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
public static final String LOCK_CHECK_DELAY = PREFIX + "lock-check-delay-seconds";
public static final ConfigOption<Long> LOCK_CHECK_DELAY_OPTION =
ConfigOptions.key(LOCK_CHECK_DELAY)
.longType()
- .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT);
+ .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT)
+ .withDescription(
+ "The delay time (in seconds) between each lock check during the rewrite operation.");
Review Comment:
@stevenzwu Thank you for your suggestion! I readed the relevant design documents and found that this configuration is used in the following four operations: `RewriteDataFiles`, `RewriteManifestFiles`, `ExpireSnapshots`, and `DeleteOrphanFiles`. I have updated the description accordingly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2286711332
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/RewriteDataFilesConfig.java:
##########
@@ -31,39 +31,73 @@
public class RewriteDataFilesConfig {
public static final String PREFIX = FlinkMaintenanceConfig.PREFIX + "rewrite.";
+ // Configuration option for maximum rewrite bytes
public static final String MAX_BYTES = PREFIX + "max-bytes";
public static final ConfigOption<Long> MAX_BYTES_OPTION =
- ConfigOptions.key(MAX_BYTES).longType().defaultValue(Long.MAX_VALUE);
+ ConfigOptions.key(MAX_BYTES)
+ .longType()
+ .defaultValue(Long.MAX_VALUE)
+ .withDescription(
+ "The maximum number of bytes allowed for the rewrite operation. "
+ + "If the total size of the data files to be rewritten exceeds this value, "
+ + "the rewrite operation will be split into multiple parts.");
Review Comment:
Thank you for your suggestion! I have refined this part based on your feedback.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2286712325
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
Review Comment:
This is a good suggestion! I have removed this comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3194911586
@pvary @Guosmilesmile @mxm @stevenzwu I’ve researched #12979, and I think this PR is a very important improvement — thank you for the great contribution!
However, I noticed a small issue: typically, Flink configurations include a description field. I believe we should add the specific meaning of the configuration to this field so that other developers can better understand its purpose when reading the code.
After looking into the code, I’ve supplemented this part of the functionality. Could you please help review this PR again? Thank you very much!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2286710375
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
public static final String LOCK_CHECK_DELAY = PREFIX + "lock-check-delay-seconds";
public static final ConfigOption<Long> LOCK_CHECK_DELAY_OPTION =
ConfigOptions.key(LOCK_CHECK_DELAY)
.longType()
- .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT);
+ .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT)
+ .withDescription(
+ "The delay time (in seconds) between each lock check during the rewrite operation.");
+ // Configuration for parallelism
+ // This setting controls the parallelism level for the maintenance tasks,
+ // determining how many tasks can run concurrently
public static final String PARALLELISM = PREFIX + "parallelism";
public static final ConfigOption<Integer> PARALLELISM_OPTION =
- ConfigOptions.key(PARALLELISM).intType().defaultValue(ExecutionConfig.PARALLELISM_DEFAULT);
+ ConfigOptions.key(PARALLELISM)
+ .intType()
+ .defaultValue(ExecutionConfig.PARALLELISM_DEFAULT)
+ .withDescription(
+ "The parallelism level for the maintenance task. "
+ + "Determines how many tasks can run concurrently.");
Review Comment:
Thank you for reviewing the code! I have updated this part accordingly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2281411573
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/RewriteDataFilesConfig.java:
##########
@@ -31,39 +31,73 @@
public class RewriteDataFilesConfig {
public static final String PREFIX = FlinkMaintenanceConfig.PREFIX + "rewrite.";
+ // Configuration option for maximum rewrite bytes
public static final String MAX_BYTES = PREFIX + "max-bytes";
public static final ConfigOption<Long> MAX_BYTES_OPTION =
- ConfigOptions.key(MAX_BYTES).longType().defaultValue(Long.MAX_VALUE);
+ ConfigOptions.key(MAX_BYTES)
+ .longType()
+ .defaultValue(Long.MAX_VALUE)
+ .withDescription(
+ "The maximum number of bytes allowed for the rewrite operation. "
+ + "If the total size of the data files to be rewritten exceeds this value, "
+ + "the rewrite operation will be split into multiple parts.");
Review Comment:
Instead of using "multiple parts," which can be ambiguous, it would be clearer to say "The rewrites for one scheduled compaction are limited in size to restrict the resources used by the compaction."
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2281301974
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
public static final String LOCK_CHECK_DELAY = PREFIX + "lock-check-delay-seconds";
public static final ConfigOption<Long> LOCK_CHECK_DELAY_OPTION =
ConfigOptions.key(LOCK_CHECK_DELAY)
.longType()
- .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT);
+ .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT)
+ .withDescription(
+ "The delay time (in seconds) between each lock check during the rewrite operation.");
Review Comment:
is it only applicable to `rewrite operation"? what about other maintenance action like "expire snapshot"?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2281302546
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -31,27 +31,50 @@ public class FlinkMaintenanceConfig {
public static final String PREFIX = "flink-maintenance.";
+ // Configuration for lock check delay (in seconds)
+ // This setting controls the delay between each lock check during the rewrite operation
public static final String LOCK_CHECK_DELAY = PREFIX + "lock-check-delay-seconds";
public static final ConfigOption<Long> LOCK_CHECK_DELAY_OPTION =
ConfigOptions.key(LOCK_CHECK_DELAY)
.longType()
- .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT);
+ .defaultValue(TableMaintenance.LOCK_CHECK_DELAY_SECOND_DEFAULT)
+ .withDescription(
+ "The delay time (in seconds) between each lock check during the rewrite operation.");
+ // Configuration for parallelism
+ // This setting controls the parallelism level for the maintenance tasks,
+ // determining how many tasks can run concurrently
public static final String PARALLELISM = PREFIX + "parallelism";
public static final ConfigOption<Integer> PARALLELISM_OPTION =
- ConfigOptions.key(PARALLELISM).intType().defaultValue(ExecutionConfig.PARALLELISM_DEFAULT);
+ ConfigOptions.key(PARALLELISM)
+ .intType()
+ .defaultValue(ExecutionConfig.PARALLELISM_DEFAULT)
+ .withDescription(
+ "The parallelism level for the maintenance task. "
+ + "Determines how many tasks can run concurrently.");
Review Comment:
seems that the second part is redundant.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on code in PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#discussion_r2286714500
##########
flink/v2.0/flink/src/main/java/org/apache/iceberg/flink/maintenance/api/FlinkMaintenanceConfig.java:
##########
@@ -66,6 +89,11 @@ public FlinkMaintenanceConfig(
this.confParser = new FlinkConfParser(table, writeOptions, readableConfig);
}
+ /**
+ * Gets the rate limit value (in seconds) for maintenance operations.
+ *
+ * @return The rate limit for maintenance operations
+ */
Review Comment:
I have made the changes according to your suggestion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "slfan1989 (via GitHub)" <gi...@apache.org>.
slfan1989 commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3226447613
@stevenzwu Could you please help review this PR again? Thank you very much!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink: Improve ConfigOption Descriptions for Flink Table Maintenance Configs. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #13832:
URL: https://github.com/apache/iceberg/pull/13832#issuecomment-3238064647
@slfan1989: Thanks for the contribution. Please don't forget to backport the changes to the other Flink branches
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org