KAFKA-20633: Update the default value of remote copy lag bytes

[kamalcph]

Summary

• Updated the default value of log.remote.copy.lag.bytes to -1
• Update documentation for remote copy lag configurations to clarify
  eligibility checks
• See KAFKA-19893: Reduce tiered storage redundancy with delayed upload (KIP-1241)  #20913 (comment)

Reviewers: Jian fujian1115@gmail.com, Luke Chen showuon@gmail.com,
Satish Duggana satishd@apache.org

[kamalcph]

@chia7712 @jiafu1115

Ping for review.

[jiafu1115]

@kamalcph LGTM. thanks. help to merge. the test should be flaky

[jiafu1115]

@kamalcph already passed the flaky test.

[kamalcph]

The PR needs one approval from committers.

@chia7712 @satishd @C0urante @showuon

can any of you stamp this PR? Thanks!

[showuon]

Will review this week or next week. Thanks.

[satishd]

Thanks @kamalcph for the PR, left a minor comment.

[showuon]

Changing the default of log.remote.copy.lag.bytes to -1 will change the existing behavior of tiered storage. If user sets [log.local.retention.ms=-1 and log.local.retention.bytes=100000], after this PR, the remote copy lag will become 100000 bytes, instead of the existing behavior: immediately. I don't think this is expected, right?

[kamalcph]

Changing the default of log.remote.copy.lag.bytes to -1 will change the existing behavior of tiered storage. If user sets [log.local.retention.ms=-1 and log.local.retention.bytes=100000], after this PR, the remote copy lag will become 100000 bytes, instead of the existing behavior: immediately. I don't think this is expected, right?

Since most users care more about time than size, we could set the default value of the time lag to 0, and the size lag to -1.

This trade-off was discussed in the thread. So, the change in behaviour is expected.

[jiafu1115]

Thanks for the discussion.
refer to code and test metric in the KIP for this case. it won't change the existed behavior when user don't change any configure due to the default lag ms is 0. the change for the default size is for user only need to change one configure instead of two configures for most of the cases (only set time as retention. also it is default configure for kafka.)

if (copyLagMs == 0 || copyLagBytes == 0) {
return true;
}

cc @kamalcph @showuon @chia7712 is there any issue for this design we missed? the original design is that two default configure items' default is 0. currently. one is -1 and another is 0 to reduce the configure effort for most of the cases.

[showuon]

Thanks for the discussion. refer to code and test metric in the KIP for this case. it won't change the existed behavior when user don't change any configure due to the default lag ms is 0. the change for the default size is for user only need to change one configure instead of two configures for most of the cases (only set time as retention. also it is default configure for kafka.)

if (copyLagMs == 0 || copyLagBytes == 0) { return true; }

cc @kamalcph @showuon @chia7712 is there any issue for this design we missed? the original design is that two default configure items' default is 0. currently. one is -1 and another is 0 to reduce the configure effort for most of the cases.

Thanks for the explanation. I re-read the KIP and previous PR, I have more understanding now. I think the point is that because lag.ms is 0, it'll be uploaded immediately no matter what lag.bytes is set. OK, looks good.

@jiafu1115 , could you please add a short section in tiered storage section to talk about the delay upload feature? It'd be good if you can mention something like what we have in log deletes section:

If both policies are enabled at the same time, a segment that is eligible for deletion due to either policy will be deleted.

This can be a separate PR. Thanks.

[showuon]

LGTM!

[kamalcph]

Thanks for the review!