Cleanup ReplicationEngine:createThreadPool to avoid hardcoding Vcr #1781
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1781 +/- ##
=============================================
- Coverage 73.82% 62.01% -11.82%
+ Complexity 9348 7792 -1556
=============================================
Files 682 688 +6
Lines 51435 52142 +707
Branches 6400 6480 +80
=============================================
- Hits 37973 32335 -5638
- Misses 11466 17744 +6278
- Partials 1996 2063 +67 Continue to review full report at Codecov.
|
| * @param threadIndexWithinPool The index of the thread within the thread pool. | ||
| * @return The name of the thread. | ||
| */ | ||
| protected String getReplicaThreadName(String datacenterToReplicateFrom, int threadIndexWithinPool) { |
There was a problem hiding this comment.
If the only difference is the prefix, the method could be getReplicaThreadPrefix() with no arguments.
There was a problem hiding this comment.
Yep, agreed. Though I named it like this on purpose to
- prevent the creation of very specialized functions
- allow the subclass needs to add more details to the thread name if needed
What do you think? I'm also okay with changing it to your suggestion.
There was a problem hiding this comment.
This is a good change. Thanks @parasgithub for making this change. I don't disagree with your argument to allow the subclasses to add more details to the thread name if needed.
However, I have a nitpick in that case. I think other than prefix, the rest of the logic is repeated code. Can you separate them out, so that it is ensured that thread naming convention doesn't change inadvertently in subclasses, unless explicitly desired.
There was a problem hiding this comment.
Done, please take a look
parasgithub
force-pushed
the
code-cleanup-branch
branch
2 times, most recently
from
6a311c6 to
2739d6a
Compare
| * @param threadIndexWithinPool The index of the thread within the thread pool. | ||
| * @return The name of the thread. | ||
| */ | ||
| public static String getCanonicalReplicaThreadName(String datacenterToReplicateTo, String datacenterToReplicateFrom, int threadIndexWithinPool) { |
There was a problem hiding this comment.
I see what you're doing here, but I don't think this method really belongs here. ThreadIndexWithinPool is a concept that only the ReplicationManager knows about, so I would move it there.
| @@ -271,6 +267,11 @@ public long getRemoteReplicaLagFromLocalInBytes(PartitionId partitionId, String | |||
| return -1; | |||
| } | |||
|
|
|||
| @Override | |||
| protected String getReplicaThreadName(String datacenterToReplicateFrom, int threadIndexWithinPool) { | |||
| return "Vcr" + ReplicaThread.getCanonicalReplicaThreadName(dataNodeId.getDatacenterName(), datacenterToReplicateFrom, threadIndexWithinPool); | |||
There was a problem hiding this comment.
This can be simply:
return "Vcr" + super. getReplicaThreadName(datacenterToReplicateFrom, threadIndexWithinPool);
And then I think the canonical logic can be collapsed back into the superclass method.
There was a problem hiding this comment.
Sorry about the delay. Made the change.
parasgithub
force-pushed
the
code-cleanup-branch
branch
3 times, most recently
from
f7458d7 to
44069a7
Compare
|
@parasgithub Do you want to take a stab at fixing the failing test? |
parasgithub
force-pushed
the
code-cleanup-branch
branch
from
44069a7 to
cb4ea1e
Compare
Done |
Sending a short pull request to make sure change is OK, will add a test afterwards (in same branch) if needed.
Issue #1742