Skip to content

Conversation

@NicoK
Copy link
Contributor

@NicoK NicoK commented Apr 18, 2018

What is the purpose of the change

Currently, floating buffers (per gate) are always required in case credit-based flow control is enabled. This, however, increases our minimum number of required network buffers.

Instead, we should also work with a minimum of zero floating buffers and set the max to the configured value. This way, if there are not enough buffers, all LocalBufferPools will at least share the available ones and not fail the job.

Brief change log

  • make credit-based input gate buffer pools flexible (0 to extraNetworkBuffersPerGate buffers, depending on availability) - this is similar to the non-credit based code path
  • clarify the documentation of taskmanager.network.memory.buffers-per-channel in this regard

Verifying this change

This change added tests and can be verified as follows:

  • added unit tests inside NetworkEnvironmentTest to cover minimum and insufficient buffers scenarios for both credit-based and non-credit based
  • add a network end-to-end test to StreamNetworkThroughputBenchmarkTest covering minimum and insufficient buffers scenarios for default, i.e. credit-based, flow control

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

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no (but per-buffer - and too few buffers may degrade performance)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? docs

@StephanEwen
Copy link
Contributor

This fix looks good, thanks!

Merging this...

@asfgit asfgit closed this in c491400 Apr 22, 2018
StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request Apr 22, 2018
sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018
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.

3 participants