INTERNAL: Integration get and mget api #860
Conversation
oliviarla
force-pushed
the
internal/getOperation
branch
2 times, most recently
from
8e0f0b5 to
6f2e9b9
Compare
| @@ -26,4 +27,9 @@ public GetOperationImpl(Collection<String> k, GetOperation.Callback c) { | |||
| setAPIType(APIType.GET); | |||
| } | |||
|
|
|||
| public GetOperationImpl(Collection<String> keys, GetOperation.Callback cb, boolean isMGet) { | |||
| super(isMGet ? CMD_MGET : CMD, cb, new HashSet<>(keys)); | |||
| setAPIType(isMGet ? APIType.MGET : APIType.GET); | |||
There was a problem hiding this comment.
APIType을 MGET으로 하여 얻는 이점이 있나요?
다른 이점이 없다면 APIType.GET으로 통일하면 될 것 같습니다.
그리고, 아래 메소드는 제거해야 하지 않나요?
public GetOperationImpl(Collection<String> k, GetOperation.Callback c) {
super(CMD, c, new HashSet<>(k));
setAPIType(APIType.GET);
}There was a problem hiding this comment.
APIType.GET으로 통일했습니다. 이제 optimize() 메서드 내부에서 APIType.MGET, APIType.MGETS 사용하지 않도록 하면 타입 자체를 제거할 수 있습니다.
말씀하신 생성자는 OptimizedGetImpl의 생성자에서 호출되어 제거할 수 없습니다..
There was a problem hiding this comment.
optimize() 메서드 내부에서 APIType.MGET, APIType.MGETS 사용하지 않도록 하고, 타입 자체를 제거하시죠.
oliviarla
force-pushed
the
internal/getOperation
branch
from
6f2e9b9 to
46702e3
Compare
| @@ -136,11 +136,9 @@ public Operation cloneMultiOperation(KeyedOperation op, MemcachedNode node, | |||
| assert !op.hasErrored() : "Attempted to clone an errored op"; | |||
|
|
|||
| if (op instanceof GetOperation) { | |||
| // If MemcachedNode supports this clone feature, it should support mget operation too. | |||
| return mget(redirectKeys, (GetOperation.Callback) mcb); | |||
| return get(redirectKeys, (GetOperation.Callback) mcb, true); | |||
There was a problem hiding this comment.
원래대로 아래와 같이 호출하는 것이 나을 것 같습니다.
return get(redirectKeys, (GetOperation.Callback) mcb, node.enabledMGetOp());There was a problem hiding this comment.
MGet 연산이 지원되는 지에 따라 mget 연산을 수행하는 것이 자연스럽습니다.
get 연산을 redirect하는 데 무조건 mget 연산을 보낸다는 것은
mget 연산이 구현된 이후에 migration 연산이 구현되었다는 history를 알고 있어야 이해가 됩니다.
나중에 조인한 인력은 왜 true일까 라고 의문을 가지게 될 것입니다.
There was a problem hiding this comment.
제 생각에는 해당 내용을 주석으로 써두면 충분할 것 같습니다.
There was a problem hiding this comment.
MGet 연산을 제공하는 Node에게 mget 명령을 요청할 수 있게 하는 것이 자연스럽고,
mget과 migration 간에 서로 관련 없을 것 같지만 실제 존재하는 어떤 의존 관계를 설명하지 않아도 됩니다.
참고로, 최근에 mget 연산을 제거할 수 있는 지를 확인하기 위해 get과 mget 연산을 비교하였습니다.
만약, mget 연산을 최신 버전에서 제거하였다면,
migration 기능이 들어간 서버에 mget 기능이 제공된다는 규칙이 깨지게 됩니다.
이러한 규칙을 가정하여 구현된 코드는 좋은 코드가 아닙니다.
| @@ -26,4 +27,9 @@ public GetOperationImpl(Collection<String> k, GetOperation.Callback c) { | |||
| setAPIType(APIType.GET); | |||
| } | |||
|
|
|||
| public GetOperationImpl(Collection<String> keys, GetOperation.Callback cb, boolean isMGet) { | |||
| super(isMGet ? CMD_MGET : CMD, cb, new HashSet<>(keys)); | |||
| setAPIType(isMGet ? APIType.MGET : APIType.GET); | |||
There was a problem hiding this comment.
optimize() 메서드 내부에서 APIType.MGET, APIType.MGETS 사용하지 않도록 하고, 타입 자체를 제거하시죠.
| @@ -1103,10 +1103,9 @@ public void complete() { | |||
|
|
|||
| Operation op; | |||
| if (node == null) { | |||
| op = opFact.mget(keyList, cb); | |||
| op = opFact.get(keyList, cb, true); | |||
There was a problem hiding this comment.
기존 코드에 대한 사소한 변경에 관한 것으로
node == null이면 isMGet = false를 넘기도록 합시다.
There was a problem hiding this comment.
기존 MGet 연산을 두는 구현보다 현재 PR 구현이 낫다고 생각하나요?
oliviarla
force-pushed
the
internal/getOperation
branch
from
46702e3 to
faba8ad
Compare
oliviarla
force-pushed
the
internal/getOperation
branch
from
faba8ad to
d535b7b
Compare
|
@uhm0311 @brido4125 |
🔗 Related Issue
⌨️ What I did
AS-IS
asyncGetBulk, asyncGetsBulk에서는 getOperation과 mgetOperation을 memcached 서버 버전에 맞게 생성하고 있습니다.
TO-BE
getOperation 만을 생성하고 안의 getOperationImpl 안에서 get과 mget을 판단하도록하여 두 APIType을 통합하였습니다.
따라서