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

Add following configs for network tuning. #1247

Merged
merged 2 commits into from
Sep 26, 2019
Merged

Conversation

zzmao
Copy link
Contributor

@zzmao zzmao commented Aug 22, 2019

routerConfig.routerMaxPutChunkNumber
networkConfig.selectorMaxRunningTimeInMs
networkConfig.selectorMaxKeyToProcess

@zzmao zzmao requested review from cgtz and jsjtzyy August 22, 2019 20:14
@codecov-io
Copy link

codecov-io commented Aug 22, 2019

Codecov Report

Merging #1247 into master will increase coverage by 0.03%.
The diff coverage is 78.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1247      +/-   ##
============================================
+ Coverage     72.35%   72.38%   +0.03%     
- Complexity     6160     6164       +4     
============================================
  Files           444      444              
  Lines         35364    35400      +36     
  Branches       4491     4499       +8     
============================================
+ Hits          25586    25624      +38     
+ Misses         8602     8601       -1     
+ Partials       1176     1175       -1
Impacted Files Coverage Δ Complexity Δ
...in/java/com.github.ambry/config/NetworkConfig.java 100% <100%> (ø) 1 <0> (ø) ⬇️
...com.github.ambry.network/NetworkClientFactory.java 100% <100%> (ø) 2 <1> (ø) ⬇️
...java/com.github.ambry.network/SSLTransmission.java 70.73% <100%> (-0.46%) 71 <2> (ø)
...in/java/com.github.ambry.network/SocketServer.java 84.36% <100%> (+0.47%) 15 <0> (ø) ⬇️
.../java/com.github.ambry.network/NetworkMetrics.java 82.05% <100%> (+0.15%) 4 <0> (ø) ⬇️
...c/main/java/com.github.ambry.network/Selector.java 78.21% <72.72%> (-1.28%) 77 <6> (+4)
.../main/java/com.github.ambry.store/ScanResults.java 81.25% <0%> (-1.57%) 16% <0%> (-1%)
.../src/main/java/com.github.ambry.store/Journal.java 90.76% <0%> (-1.54%) 24% <0%> (-1%)
...ava/com.github.ambry.router/NonBlockingRouter.java 77.26% <0%> (-0.52%) 51% <0%> (-1%)
... and 6 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 76fc882...c1cfd28. Read the comment docs.

*/
@Config("router.max.put.chunk.number")
@Default("4")
public final int routerMaxPutChunkNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

I ended up adding this config in another PR, so you can remove this

VerifiableProperties vprops = new VerifiableProperties(props);
NetworkConfig networkConfig = new NetworkConfig(vprops);
this.selector =
new Selector(new NetworkMetrics(new MetricRegistry()), SystemTime.getInstance(), null, networkConfig);
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to test the new configs?

@@ -340,6 +346,10 @@ private void pollOnMainThread(long timeoutMs, List<NetworkSend> sends) throws IO
metrics.selectorReadyKeyCount.inc(keys.size());
Iterator<SelectionKey> iter = keys.iterator();
while (iter.hasNext()) {
if (networkConfig.selectorMaxRunningTimeInMs != -1
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like one of these configs applies only for main thread polling and one applies for executor pool polling

@zzmao
Copy link
Contributor Author

zzmao commented Sep 17, 2019

PR updated. Feel free to review. Working on a test per @cgtz 's comment, will push it soon.

Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

Looks good. Two minor comments.

metrics.selectorIOCount.inc();
metrics.selectorIOTime.update(time.milliseconds() - endSelect);
long selectorIOTime = time.milliseconds() - endSelect;
if (selectorIOTime >= 50) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We choose 50 here based on observation? Also, I feel like selectorIOTime can be updated before checkUnreadyConnectionsStatus.

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, it's based on observation to filter out small selectorIOTime that we don't care about.
pollOnMainThread also do the same thing for selectorIOTime. Prefer not to change it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checkUnreadyConnectionsStatus is just some local computation. It should be a fast call.

@@ -66,6 +67,7 @@
private SSLFactory sslFactory;

public SocketServer(NetworkConfig config, SSLConfig sslConfig, MetricRegistry registry, ArrayList<Port> portList) {
this.networkConfig = config;
this.host = config.hostName;
this.port = config.port;
this.numProcessorThreads = config.numIoThreads;
Copy link
Contributor

Choose a reason for hiding this comment

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

selectorExecutorPoolSize can be removed from this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed.

Copy link
Contributor

@jsjtzyy jsjtzyy left a comment

Choose a reason for hiding this comment

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

LGTM

@zzmao
Copy link
Contributor Author

zzmao commented Sep 26, 2019

@cgtz Good to merge this?

@cgtz cgtz merged commit c0d9028 into linkedin:master Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants