gharris1727 commented on code in PR #16080:
URL: https://github.com/apache/kafka/pull/16080#discussion_r1624808530
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConfig.java:
##########
@@ -101,7 +101,11 @@ public class MirrorSourceConfig extends
MirrorConnectorConfig {
public static final Class<?> CONFIG_PROPERTY_FILTER_CLASS_DEFAULT =
DefaultConfigPropertyFilter.class;
public static final String OFFSET_LAG_MAX = "offset.lag.max";
- private static final String OFFSET_LAG_MAX_DOC = "How out-of-sync a remote
partition can be before it is resynced.";
+ private static final String OFFSET_LAG_MAX_DOC = "Determines the maximum
allowed lag between the source and target partitions before MirrorMaker emits
an 'offset sync' event to catch up the remote partition. The lag is calculated
as the difference between the latest offset in the source partition, and the
last sync emitted to the offset syncs topic. " +
+ "When the lag for a remote partition exceeds the
<code>offset.lag.max</code> value, MirrorMaker emits an 'offset sync' event to
the 'offset-syncs' topic, which is then used by the
<code>MirrorCheckpointTask</code> to translate source and target partitions." +
Review Comment:
please edit this to combine redundant sentences.
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConfig.java:
##########
@@ -101,7 +101,11 @@ public class MirrorSourceConfig extends
MirrorConnectorConfig {
public static final Class<?> CONFIG_PROPERTY_FILTER_CLASS_DEFAULT =
DefaultConfigPropertyFilter.class;
public static final String OFFSET_LAG_MAX = "offset.lag.max";
- private static final String OFFSET_LAG_MAX_DOC = "How out-of-sync a remote
partition can be before it is resynced.";
+ private static final String OFFSET_LAG_MAX_DOC = "Determines the maximum
allowed lag between the source and target partitions before MirrorMaker emits
an 'offset sync' event to catch up the remote partition. The lag is calculated
as the difference between the latest offset in the source partition, and the
last sync emitted to the offset syncs topic. " +
+ "When the lag for a remote partition exceeds the
<code>offset.lag.max</code> value, MirrorMaker emits an 'offset sync' event to
the 'offset-syncs' topic, which is then used by the
<code>MirrorCheckpointTask</code> to translate source and target partitions." +
+ "Setting <code>offset.lag.max</code> to a lower value can be
beneficial in scenarios where partitions have consistently low throughput, and
the fixed sync interval ('offset.flush.interval.ms') is not acceptable. A lower
value will trigger more frequent offset syncs, keeping the target partitions
more closely in sync with the source for these low-throughput partitions. " +
+ "On the other hand, setting it to a higher value can be useful
when the source topic has high throughput and the remote partitions can
tolerate a larger lag without impacting target costumers." +
+ "It's also possible to set <code>offset.lag.max</code> to 0, which
will cause MirrorMaker to emit an 'offset sync' for every source record to a
target partition. This can be useful for strict synchronization requirements
but may increase the load and extra throughput on the 'offset-syncs' topic,
depending on the value of the 'offset-syncs.topic.location' configuration";
Review Comment:
Don't use "strict synchronization" here, as it could be confused for
"synchronous", which is a capability that MM2 does not have.
Also the mention of the offset-syncs.topic.location needs to be rephrased.
If you say "X may do Y, depending on the value of Z", it makes me think there
is some value of Z avoids Y, when that isn't the case.
Lowering the value _will_ increase the load on the offset-syncs topic except
in very specific scenarios, so using _may_ is more confusing than it's worth.
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConfig.java:
##########
@@ -101,7 +101,11 @@ public class MirrorSourceConfig extends
MirrorConnectorConfig {
public static final Class<?> CONFIG_PROPERTY_FILTER_CLASS_DEFAULT =
DefaultConfigPropertyFilter.class;
public static final String OFFSET_LAG_MAX = "offset.lag.max";
- private static final String OFFSET_LAG_MAX_DOC = "How out-of-sync a remote
partition can be before it is resynced.";
+ private static final String OFFSET_LAG_MAX_DOC = "Determines the maximum
allowed lag between the source and target partitions before MirrorMaker emits
an 'offset sync' event to catch up the remote partition. The lag is calculated
as the difference between the latest offset in the source partition, and the
last sync emitted to the offset syncs topic. " +
+ "When the lag for a remote partition exceeds the
<code>offset.lag.max</code> value, MirrorMaker emits an 'offset sync' event to
the 'offset-syncs' topic, which is then used by the
<code>MirrorCheckpointTask</code> to translate source and target partitions." +
+ "Setting <code>offset.lag.max</code> to a lower value can be
beneficial in scenarios where partitions have consistently low throughput, and
the fixed sync interval ('offset.flush.interval.ms') is not acceptable. A lower
value will trigger more frequent offset syncs, keeping the target partitions
more closely in sync with the source for these low-throughput partitions. " +
+ "On the other hand, setting it to a higher value can be useful
when the source topic has high throughput and the remote partitions can
tolerate a larger lag without impacting target costumers." +
Review Comment:
You don't need to say "without impacting...", plus there is a spelling
mistake there. This can be combined with the previous discussion on lowering
the value.
##########
connect/mirror/src/main/java/org/apache/kafka/connect/mirror/MirrorSourceConfig.java:
##########
@@ -101,7 +101,11 @@ public class MirrorSourceConfig extends
MirrorConnectorConfig {
public static final Class<?> CONFIG_PROPERTY_FILTER_CLASS_DEFAULT =
DefaultConfigPropertyFilter.class;
public static final String OFFSET_LAG_MAX = "offset.lag.max";
- private static final String OFFSET_LAG_MAX_DOC = "How out-of-sync a remote
partition can be before it is resynced.";
+ private static final String OFFSET_LAG_MAX_DOC = "Determines the maximum
allowed lag between the source and target partitions before MirrorMaker emits
an 'offset sync' event to catch up the remote partition. The lag is calculated
as the difference between the latest offset in the source partition, and the
last sync emitted to the offset syncs topic. " +
+ "When the lag for a remote partition exceeds the
<code>offset.lag.max</code> value, MirrorMaker emits an 'offset sync' event to
the 'offset-syncs' topic, which is then used by the
<code>MirrorCheckpointTask</code> to translate source and target partitions." +
+ "Setting <code>offset.lag.max</code> to a lower value can be
beneficial in scenarios where partitions have consistently low throughput, and
the fixed sync interval ('offset.flush.interval.ms') is not acceptable. A lower
value will trigger more frequent offset syncs, keeping the target partitions
more closely in sync with the source for these low-throughput partitions. " +
Review Comment:
The only part of this line that is worth including is "A lower value will
trigger more frequent offset syncs".
What I meant in the previous comment was that 100 is already so close to 0
that nobody will realistically lower this value, so we don't need to describe
it in much detail.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
