jiafu1115 commented on code in PR #20913:
URL: https://github.com/apache/kafka/pull/20913#discussion_r3205197265
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogConfig.java:
##########
@@ -138,6 +144,10 @@ public Optional<String> serverConfigName(String
configName) {
public static final boolean DEFAULT_REMOTE_LOG_DELETE_ON_DISABLE_CONFIG =
false;
public static final long DEFAULT_LOCAL_RETENTION_BYTES = -2; // It
indicates the value to be derived from RetentionBytes
public static final long DEFAULT_LOCAL_RETENTION_MS = -2; // It indicates
the value to be derived from RetentionMs
+ public static final long DEFAULT_REMOTE_COPY_LAG_MS = -2; // It indicates
no delay check based on local retention ms
+ public static final long DEFAULT_REMOTE_COPY_LAG_BYTES = -2; // It
indicates no delay check based on local retention bytes
Review Comment:
@kamalcph Hi
Your case inspired me to decide to drop the -1/-2 options in this phase to
reduce the code complexity. Thank you very much.
I already done with all of refactor based on this PR directly (you can
ignore all the discussion I post before this reply) and I also add more unit
tests.
You can help to take second pass review now. Thanks a lot.
Some tips:
1 I don't have -1 now though I think it is valuable. but if we need it we
can do it in future.
2 I don't add the Long.MaxValue as the special logic. Instead of it I keep
the validation: lag delay value <= local retention value
3 We still need logic to distinguish follow case:
This case: I want to set time-based lag when no limit for size retention
retention.ms = 2 days
local.retention.ms = 6 hour
retention.bytes = -1 no limit by default
local.retention.bytes = -2 no limit by default
remote.copy.lag.ms = 5.5 hour
remote.copy.lag.size= what's the default value here ？ I leave it as
0/default value. It mean no check based on size. need to check time as the
final result
This case you raised: Not breaking the delete policy
retention.ms = 2 days
local.retention.ms = 6 hour
retention.bytes = 10 GB
local.retention.ms = 5 GB
remote.copy.lag.ms = 5.5 hour
remote.copy.lag.size = what's the default value here: I leave it as
0/default value. upload at once no matter time related configure to avoid
breaking the size-based retention.
So I update the logic and document as followed:
"When set to 0, immediate upload when local time-based retention is used;
otherwise no time-based delay check.
Thanks
--
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]
