CLEANUP: Refactored MemcachedConnection.switchoverMemcachedReplGroup() method.#852
Merged
jhpark816 merged 1 commit intonaver:developfrom Dec 19, 2024
Merged
CLEANUP: Refactored MemcachedConnection.switchoverMemcachedReplGroup() method.#852jhpark816 merged 1 commit intonaver:developfrom
MemcachedConnection.switchoverMemcachedReplGroup() method.#852jhpark816 merged 1 commit intonaver:developfrom
Conversation
uhm0311
force-pushed
the
uhm0311/repl
branch
from
fd4eeeb to
fbae49b
Compare
oliviarla
reviewed
Dec 17, 2024
| } else { | ||
| getLogger().warn("Delay switchover because invalid group state : " + group); | ||
| } | ||
| ((ArcusReplKetamaNodeLocator) locator).switchoverReplGroup(group); |
Collaborator
There was a problem hiding this comment.
기존처럼 group.getMasterNode() != null && group.getMasterCandidate() != null 조건이 만족할 때에만 switchover + move operation + old master reconnect 해야 하지 않나요?
Collaborator
Author
There was a problem hiding this comment.
https://github.com/jam2in/arcus-works/issues/637#issuecomment-2521925920
해당 조건에 맞지 않는데도 이 메소드를 호출하는 경우가 로직적으로는 없어서 조건문을 제거했습니다.
그래도 조건문을 유지하도록 할까요?
Collaborator
There was a problem hiding this comment.
private 메서드라서 반드시 유지할 필요는 없어보이지만, 단순 검증 용도로 둔다면 나중에 리팩토링 시에 이 부분을 인지하여 버그 발생 가능성이 줄어들 것 같습니다.
Collaborator
There was a problem hiding this comment.
아래와 같이 조건을 넣어 두시죠.
- oldMaster는 분명히 존재하는 상황에서 switchover 대상이 될 수 있음.
- masterCandidate가 null인 경우는 없을 것으로 보이지만,
masterCandidate가 null인 경우에 locator.switchoverReplGroup(group) 수행은
오동작(invalid group으로 변환)하게 되므로,
masterCandidate != null 조건을 넣어두는 것이 괜찮을 것 같습니다.
if (group.getMasterCandidate() != null) {
((ArcusReplKetamaNodeLocator) locator).switchoverReplGroup(group);
. . .
queueReconnect(oldMaster, ReconnDelay.IMMEDIATE,
"Discarded all pending reading state operation to move operations.");
} else {
getLogger().warn("Delay switchover because invalid group state : " + group);
}
oliviarla
previously approved these changes
Dec 17, 2024
jhpark816
reviewed
Dec 17, 2024
|
|
||
| private void handleReads(MemcachedNode qa) | ||
| throws IOException { | ||
| MemcachedReplicaGroup group = qa.getReplicaGroup(); |
Collaborator
There was a problem hiding this comment.
복제하지 않는 경우에서 위의 코드는 불필요합니다.
group 변수가 실제로 필요한 곳으로 위의 코드를 옮기면 좋겠습니다.
| qa.getReplicaGroup().masterNode == qa) { | ||
| delayedSwitchoverGroups.remove(qa.getReplicaGroup()); | ||
| switchoverMemcachedReplGroup(qa, false); | ||
| if (group.isDelayedSwitchover() && group.masterNode == qa) { |
Collaborator
There was a problem hiding this comment.
group.masterNode 대신에 group.getMasterNode() 호출합시다.
| MemcachedNode newMaster = group.getMasterNode(); | ||
|
|
||
| oldMaster.moveOperations(newMaster, cancelNonIdempotent); | ||
| addedQueue.offer(group.getMasterNode()); |
Collaborator
There was a problem hiding this comment.
addedQueue.offer(newMaster); 이면 됩니다.
| } else { | ||
| getLogger().warn("Delay switchover because invalid group state : " + group); | ||
| } | ||
| ((ArcusReplKetamaNodeLocator) locator).switchoverReplGroup(group); |
Collaborator
There was a problem hiding this comment.
아래와 같이 조건을 넣어 두시죠.
- oldMaster는 분명히 존재하는 상황에서 switchover 대상이 될 수 있음.
- masterCandidate가 null인 경우는 없을 것으로 보이지만,
masterCandidate가 null인 경우에 locator.switchoverReplGroup(group) 수행은
오동작(invalid group으로 변환)하게 되므로,
masterCandidate != null 조건을 넣어두는 것이 괜찮을 것 같습니다.
if (group.getMasterCandidate() != null) {
((ArcusReplKetamaNodeLocator) locator).switchoverReplGroup(group);
. . .
queueReconnect(oldMaster, ReconnDelay.IMMEDIATE,
"Discarded all pending reading state operation to move operations.");
} else {
getLogger().warn("Delay switchover because invalid group state : " + group);
}
uhm0311
force-pushed
the
uhm0311/repl
branch
from
fbae49b to
1815502
Compare
jhpark816
approved these changes
Dec 19, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issue
⌨️ What I did
MemcachedNode에서MemcachedReplicaGroup으로 변경합니다.