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

Use void promise when sending #1062

Merged
merged 2 commits into from
Dec 19, 2017
Merged

Use void promise when sending #1062

merged 2 commits into from
Dec 19, 2017

Conversation

no2chem
Copy link
Member

@no2chem no2chem commented Dec 12, 2017

Overview

Description: When calling write or writeAndFlush, our RPC layer isn't interested in the completion of the write. By using voidPromise(), we inform Netty that we aren't interested and resources don't need to be help pending the completion of the write.

Why should this be merged: This very minor change should reduce memory pressure and slightly improve performance because Netty no longer needs to track completions.

Checklist (Definition of Done):

  • There are no TODOs left in the code
  • Coding conventions (e.g. for logging, unit tests) have been followed
  • Change is covered by automated tests
  • Public API has Javadoc

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 96a56af on NettyVoidPromise into ** on master**.

@corfudb-performance
Copy link
Collaborator

Results automatically generated by CorfuDB Benchmark Framework to assess the performance of this pull request for commit 96a56af.

*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx+Scan workload, 10 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 1 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 5 threads, Disk mode
*** 0.0% transaction FAILURE rate for NonConflictingTx workload, 10 threads, Disk mode

An interactive dashboard with Pull Request Performance Metrics for ALL cluster types and numbers of threads in run, is available at:
Pull Request #1062 Graphs

Copy link
Contributor

@Maithem Maithem left a comment

Choose a reason for hiding this comment

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

Less generated garbage. Good.
Just rebase.

@corfu-repo
Copy link
Collaborator

SonarQube analysis reported 1 issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. BLOCKER NettyClientRouter.java#L447: Replace the call to "Thread.sleep(...)" with a call to "wait(...)". rule

@codecov
Copy link

codecov bot commented Dec 19, 2017

Codecov Report

Merging #1062 into master will increase coverage by 0.17%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1062      +/-   ##
==========================================
+ Coverage   65.62%   65.79%   +0.17%     
==========================================
  Files         214      214              
  Lines        9970     9970              
  Branches      985      985              
==========================================
+ Hits         6543     6560      +17     
+ Misses       3043     3034       -9     
+ Partials      384      376       -8
Impacted Files Coverage Δ
.../org/corfudb/infrastructure/NettyServerRouter.java 54.66% <100%> (ø) ⬆️
...org/corfudb/runtime/clients/NettyClientRouter.java 71.21% <25%> (ø) ⬆️
.../org/corfudb/protocols/wireprotocol/IMetadata.java 84.05% <0%> (-2.9%) ⬇️
...main/java/org/corfudb/runtime/view/LayoutView.java 85.6% <0%> (+0.75%) ⬆️
.../runtime/view/stream/AbstractQueuedStreamView.java 80.98% <0%> (+1.4%) ⬆️
...a/org/corfudb/infrastructure/ManagementServer.java 78.16% <0%> (+2.29%) ⬆️
...runtime/view/stream/AbstractContextStreamView.java 64.28% <0%> (+2.38%) ⬆️
...va/org/corfudb/runtime/view/SealServersHelper.java 82.6% <0%> (+2.89%) ⬆️
.../infrastructure/management/PeriodicPollPolicy.java 92.3% <0%> (+3.29%) ⬆️
...g/corfudb/runtime/object/StreamViewSMRAdapter.java 66.66% <0%> (+3.92%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1b680a...4291ea6. Read the comment docs.

@no2chem no2chem merged commit 08af8d8 into master Dec 19, 2017
@no2chem no2chem deleted the NettyVoidPromise branch December 21, 2017 00:21
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.

None yet

5 participants