Skip to content

Conversation

@yjshen
Copy link
Member

@yjshen yjshen commented Dec 3, 2019

Motivation

Currently, while using round-robin message router, messages would be route to different partitions at the maxPublishDelay boundaries. However, when GC occurs, the output partition would change frequently and results in small batches.

This PR introduces a conf field setting the round-robin partition switch frequency: batchingPartitionSwitchFrequencyByPublishDelay. With this PR, the partition switch interval could be set as maxPublishDelay * frequency.

Modifications

batchingPartitionSwitchFrequencyByPublishDelay in ProducerConfiguratiionData.java

Verifying this change

This change is already covered by existing tests.

Does this pull request potentially affect one of the following parts:

  • The public API: (yes)

Documentation

  • Does this pull request introduce a new feature? (yes )
  • If yes, how is the feature documented? (docs / JavaDocs)

* @see #messageRoutingMode(MessageRoutingMode)
* @see #batchingMaxPublishDelay(long, TimeUnit)
*/
ProducerBuilder<T> batchingPartitionSwitchFrequency(int frequency);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to add both time interval and time unit in the method, by following the convention of other time related settings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current frequency is used to compute switch delay in frequency * maxBatchDelayMicros. Do you mean I should make it a standalone variable? Then if users didn't set this variable, or the user is setting a variable less than maxBatchDelay, I need to use maxBatchDelay in round-robin partitioner?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I misread the code. I see it now. Then that's fine to me.

private MessageRouter customMessageRouter = null;

private long batchingMaxPublishDelayMicros = TimeUnit.MILLISECONDS.toMicros(1);
private int batchingPartitionSwitchFrequencyByPublishDelay = 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add "Micros" to the name so people know the time unit directly by reading the name.

Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good me, is it possible implement in RoundRobinPartitionMessageRouterImpl? Looks the batching partition switch frequency only works in RoundRobin mode. Please considering whether this configuration should in ProducerBuilder.

@yjshen
Copy link
Member Author

yjshen commented Dec 4, 2019

The change looks good me, is it possible implement in RoundRobinPartitionMessageRouterImpl? Looks the batching partition switch frequency only works in RoundRobin mode. Please considering whether this configuration should in ProducerBuilder.

I think we are exposing the capability of changing partition switch frequency to users? If so, the user conf in RoundRobinPartitionMessageRouterImpl should be fetched from ProducerConfigurationData?

@codelipenghui
Copy link
Contributor

@sijie I have discussed with yijie, i'm not sure use a custom router is reasonable here. we can support setting frequency in RoundRobinPartitionMessageRouterImpl and user use it like:

newProducer().router(new RoundRobinPartitionMessageRouterImpl(int frequency))

Which can reduces ProducerConfigurationData complexity since we all ready have routeMode and customRouter there.

@sijie
Copy link
Member

sijie commented Dec 6, 2019

@codelipenghui that works for me.

@codelipenghui
Copy link
Contributor

run java8 tests

@yjshen
Copy link
Member Author

yjshen commented Dec 7, 2019

run java8 tests.
run integration tests.

@codelipenghui codelipenghui merged commit 6c47ba8 into apache:master Dec 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants