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.

Sign up for GitHub

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

Jump to bottom

[FLINK-12201][network,metrics] Introduce InputGateWithMetrics in Task to increment numBytesIn metric #8320

Merged
merged 3 commits into from Jun 4, 2019
Merged

[FLINK-12201][network,metrics] Introduce InputGateWithMetrics in Task to increment numBytesIn metric #8320

merged 3 commits into from Jun 4, 2019

Conversation

zhijiangW
Copy link
Contributor

@zhijiangW zhijiangW commented Apr 30, 2019 •
edited

What is the purpose of the change

  • Incrementing of numBytesIn metric in SingleInputGate does not depend on shuffle service and can be moved out of network internals into Task. Task could wrap InputGate provided by ShuffleService with InputGateWithMetrics which would increment numBytesIn metric.*

Brief change log

  • Introduce InputGateWithMetrics to wrap InputGate from ShuffleService in Task
  • Remove the counter from SingleInputGate and related creation.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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

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

Documentation

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

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 30, 2019 •
edited

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

  • ✅ 1. The [description] looks good.
  • ✅ 2. There is [consensus] that the contribution should go into to Flink.
  • ❗ 3. Needs [attention] from.
  • ✅ 4. The change fits into the overall [architecture].
  • ✅ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@zhijiangW
Copy link
Contributor Author

@flinkbot attention @azagrebin

azagrebin
azagrebin reviewed Apr 30, 2019
View reviewed changes
Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

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

Thanks for this refactoring @zhijiangW ! It looks good overall to me, I've left couple of small comments.

...time/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/BufferOrEvent.java Outdated Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java Show resolved Hide resolved
@zhijiangW zhijiangW force-pushed the FLINK-12201 branch 5 times, most recently from 309f321 to 0500cee Compare May 9, 2019 07:23
@zhijiangW
Copy link
Contributor Author

@azagrebin I rebased the codes to not rely on the commit of #8310 and made some changes.
Could you help double review this PR? :)

@azagrebin
Copy link
Contributor

@flinkbot approve all

azagrebin
azagrebin reviewed May 10, 2019
View reviewed changes
Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

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

Thanks @zhijiangW ! I've left some comments.

flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/InputGateWithMetrics.java Outdated Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/InputGateWithMetrics.java Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/InputGateWithMetrics.java Outdated Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/InputGateWithMetrics.java Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/InputGateWithMetrics.java Outdated Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java Outdated Show resolved Hide resolved
@zhijiangW zhijiangW changed the title [FLINK-12201][network][metrics] Introduce InputGateWithMetrics in Task to increment numBytesIn metric [FLINK-12201][network][metrics] Refactor the metric of numBytesIn out of SingleInputGate May 23, 2019
@zhijiangW
Copy link
Contributor Author

@azagrebin I have rebased the master to solve the conflicts.

@zhijiangW zhijiangW changed the title [FLINK-12201][network][metrics] Refactor the metric of numBytesIn out of SingleInputGate [FLINK-12201][network,metrics] Introduce InputGateWithMetrics in Task to increment numBytesIn metric May 24, 2019
azagrebin
azagrebin requested changes May 28, 2019
View reviewed changes
Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

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

Thanks @zhijiangW ! It looks quite good to me, I left some smaller comments. Especially about benchmark which we should run before merge.

...time/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/BufferOrEvent.java Outdated Show resolved Hide resolved
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/Task.java Outdated Show resolved Hide resolved
.../java/org/apache/flink/streaming/runtime/io/benchmark/StreamNetworkBenchmarkEnvironment.java Show resolved Hide resolved
@zhijiangW
Copy link
Contributor Author

Thanks for further reviews @azagrebin !

I pushed one fixup commit for addressing above comments! And I would submit the micro benchmark comparison later.

@zhijiangW
Copy link
Contributor Author

I have squashed the commits for addressing the comments. @azagrebin

@zhijiangW zhijiangW force-pushed the FLINK-12201 branch 2 times, most recently from 92bdba5 to fc5d18f Compare May 31, 2019 02:23
pnowojski
pnowojski requested changes May 31, 2019
View reviewed changes
Copy link
Contributor

@pnowojski pnowojski left a comment

Choose a reason for hiding this comment

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

Thanks for the change @zhijiangW. I've left couple of comments.

FYI, this PR will have minor conflicts with my changes in #8476 . Just in case my PR will be merged before, I've made some minor changes to the InputGate interface.

flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/InputGateWithMetrics.java Outdated Show resolved Hide resolved
...time/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/BufferOrEvent.java Outdated Show resolved Hide resolved
...me/src/main/java/org/apache/flink/runtime/io/network/partition/consumer/SingleInputGate.java Outdated Show resolved Hide resolved
.../java/org/apache/flink/streaming/runtime/io/benchmark/StreamNetworkBenchmarkEnvironment.java Show resolved Hide resolved
@zhijiangW
[FLINK-12201][network,metrics] Introduce InputGateWithMetrics in Task…
ad0886b 
… to increment numBytesIn metric

Incrementing of numBytesIn metric in SingleInputGate does not depend on shuffle service and can be moved out of network
internals into Task. Task could wrap InputGate provided by ShuffleService with InputGateWithMetrics which would increment
numBytesIn metric.
pnowojski
pnowojski approved these changes Jun 4, 2019
View reviewed changes
Copy link
Contributor

@pnowojski pnowojski left a comment •
edited

Choose a reason for hiding this comment

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

LGTM merging @zhijiangW let me know once you fix the typo in the commit, I'll merge it then.

azagrebin
azagrebin approved these changes Jun 4, 2019
View reviewed changes
Copy link
Contributor

@azagrebin azagrebin left a comment

Choose a reason for hiding this comment

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

Thanks @zhijiangW ! LGTM

@zhijiangW
[hotfix][network] Drop redundant getSizeUnsafe from Buffer interface
5474cc2 
The implementations of getSize and getSizeUnsafe are exactly the same now, which do not need the synchronized way.
So We could remove the getSizeUnsafe to make it clean and clear.
@pnowojski pnowojski merged commit a36ec7f into apache:master Jun 4, 2019
@zhijiangW zhijiangW deleted the FLINK-12201 branch June 10, 2020 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pnowojski pnowojski pnowojski approved these changes

@azagrebin azagrebin azagrebin approved these changes

Assignees
No one assigned
Labels
component=Runtime/Network review=description?
Projects
None yet
Milestone
No milestone
5 participants
@zhijiangW @flinkbot @azagrebin @pnowojski @rmetzger