Skip to content

Conversation

@greghogan
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

This method preserves the behavior of FORWARD only if numberOfChannels is always 1 when FORWARD is configured which is out of control of the OutputEmitter.
Should we add a check somewhere (e.g., in this condition to reduce the overhead) to ensure that numberOfChannels is acutally 1 for FORWARD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did overlook that, though customPartition does likewise and the OutputEmitter will only be used for a single strategy. The strategy is set in the constructor so the channel list can be created there for the types which do not broadcast.

@greghogan greghogan force-pushed the 2897_use_distinct_initial_indices_for_outputemitter_round_robin branch from 6365e69 to 699009f Compare October 23, 2015 14:06
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this case be also covered by nextChannel %= numberOfChannels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modulo computation is executed in two cases: 1) the initial call to robin when the source parallelism is greater than the receiver parallelism and 2) when the receiver parallelism is dynamically changing.

Unoptimized modulo is much slower than a test for equality but is required in the first case to evenly distribute the first outputs.

@StephanEwen
Copy link
Contributor

Aside from till's comment (simplification of the "robin()" case), this looks good.

@tillrohrmann
Copy link
Contributor

Yes, +1 for merging. Thanks for your contribution @greghogan.

@StephanEwen
Copy link
Contributor

Will merge this...

StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Dec 7, 2015
@asfgit asfgit closed this in 22ac65b Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants