Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-8212] [network] Pull EnvironmentInformation out of TaskManager… #5458

Closed
wants to merge 1 commit into from

Conversation

rice668
Copy link

@rice668 rice668 commented Feb 12, 2018

What is the purpose of the change

Pull EnvironmentInformation out of TaskManagerServices

Brief change log

Add the required information to fromConfiguration method in TaskManagerRunner.java

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): ( don't know)
  • 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? (not documented)

@rice668 rice668 force-pushed the flink-8212 branch 2 times, most recently from 1e744b0 to cb593fc Compare February 14, 2018 10:19
@rice668
Copy link
Author

rice668 commented Feb 14, 2018

@tillrohrmann I updated the code. Could you take a look ? Thanks ~

private static MemoryManager createMemoryManager(TaskManagerServicesConfiguration taskManagerServicesConfiguration) throws Exception {
private static MemoryManager createMemoryManager(TaskManagerServicesConfiguration taskManagerServicesConfiguration,
long freeHeapMemoryWithDefrag,
long maxJvmHeapMemory) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention is to break parameter lists like

public void a(
        A a,
        B b) {
    // foobar
}

Copy link
Author

Choose a reason for hiding this comment

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

Will fix ~

*
* @return memory to use for network buffers (in bytes)
*/
public static long calculateNetworkBufferMemory(TaskManagerServicesConfiguration tmConfig) {
public static long calculateNetworkBufferMemory(TaskManagerServicesConfiguration tmConfig,
long maxJvmHeapMemory) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here with the parameters

Copy link
Author

Choose a reason for hiding this comment

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

Will fix ~

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Changes look good to me @zhangminglei. Thanks for your contribution @zhangminglei. Merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants