[Draft] Changes for having replication thread per remote host #2715
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2715 +/- ##
=============================================
- Coverage 70.71% 52.89% -17.82%
+ Complexity 11561 7915 -3646
=============================================
Files 830 830
Lines 70675 70719 +44
Branches 8499 8508 +9
=============================================
- Hits 49978 37410 -12568
- Misses 18074 30295 +12221
- Partials 2623 3014 +391 ☔ View full report in Codecov by Sentry. |
| if (numOfThreadsInPool <= 0) { | ||
| return null; | ||
| } | ||
| if (!replicationConfig.replicationUseSeparateThreadForEachHost) { |
There was a problem hiding this comment.
flip the order of the if-else; if (use-thread-per-host) {} else {}
| datacenter.equals(dataNodeId.getDatacenterName()) ? replicationConfig.replicationNumOfIntraDCReplicaThreads | ||
| : replicationConfig.replicationNumOfInterDCReplicaThreads; | ||
| if (numOfThreadsInPool <= 0) { | ||
| return null; |
| logger.warn("Number of replica threads is smaller or equal to 0, not starting any replica threads for {}.", | ||
| datacenter); | ||
| ReplicaThread replicaThread = getReplicaThreadForRemoteReplica(remoteReplicaInfo, startThread); | ||
| if (replicaThread == null) { |
There was a problem hiding this comment.
if (replicaThread != null) { do_something }
| * @param startThread If thread needs to be started when created. | ||
| * @return {@link ReplicaThread} created for the remote node | ||
| */ | ||
| private ReplicaThread createThread(DataNodeId dataNodeId, boolean startThread) { |
There was a problem hiding this comment.
I think this should solve the issue since its usually a slow host, and not a slow partition on the host affecting repl
| */ | ||
| @Config("replication.use.separate.thread.for.each.host") | ||
| @Default("true") | ||
| public final boolean replicationUseSeparateThreadForEachHost; |
There was a problem hiding this comment.
instead of boolean, use an int to indicate num threads per host
0 = go the old way (default)
1 or more = 1 or more threads per host
| if (numOfThreadsInPool <= 0) { | ||
| return null; | ||
| } | ||
| if (!replicationConfig.replicationUseSeparateThreadForEachHost) { |
There was a problem hiding this comment.
the order of conditions here will dictate what we prioritize - thread-per-host or the old-way
| return dataNodeIdToReplicaThread.computeIfAbsent(dataNodeIdToReplicate, | ||
| key -> replicaThreads.get(getReplicaThreadIndexToUse(datacenter))); | ||
| } else { | ||
| return dataNodeIdToReplicaThread.computeIfAbsent(dataNodeIdToReplicate, |
There was a problem hiding this comment.
if we have more than one thread per remote host, use round robin to assign replicas on that remote host to threads on local host
| @@ -305,135 +305,135 @@ public ReplicationMetrics(MetricRegistry registry, List<? extends ReplicaId> rep | |||
| public void populateSingleColoMetrics(String datacenter) { | |||
| Meter interColoReplicationBytesRatePerDC = | |||
| registry.meter(MetricRegistry.name(ReplicaThread.class, "Inter-" + datacenter + "-ReplicationBytesRate")); | |||
| interColoReplicationBytesRate.put(datacenter, interColoReplicationBytesRatePerDC); | |||
| interColoReplicationBytesRate.putIfAbsent(datacenter, interColoReplicationBytesRatePerDC); | |||
There was a problem hiding this comment.
just get rid these statements pls
| * If true, replication Get requests asks for deleted and expired blobs as well to succeed in scenarios where blobs | ||
| * get deleted or expired after replication metadata exchange. | ||
| */ | ||
| @Config("replication.use.separate.thread.for.each.host") |
There was a problem hiding this comment.
use public static final in CAPS for this string replication.use.separate.thread.for.each.host
| * get deleted or expired after replication metadata exchange. | ||
| */ | ||
| @Config("replication.use.separate.thread.for.each.host") | ||
| @Default("true") |
There was a problem hiding this comment.
remove this default, not used anywhere
| if (numOfThreadsInPool <= 0) { | ||
| return null; | ||
| } | ||
| protected List<ReplicaThread> getOrCreateThreadPoolIfNecessary(String datacenter, boolean startThread, |
There was a problem hiding this comment.
just rm this fn and merge the code above in its call site
| */ | ||
| @Config("replication.use.separate.thread.for.each.host") | ||
| @Default("true") | ||
| public final boolean replicationUseSeparateThreadForEachHost; |
There was a problem hiding this comment.
replicationUseSeparateThreadForEachHost -> numReplicationThreadsPerRemoteHost
|
@snalli I haven't actually finished working on this pr. Want to do some WIP tests in EI to see if this change helps. That's why I put it in draft status and didn't add any reviewers :) But, thanks for initial comments! |
No description provided.