-
Notifications
You must be signed in to change notification settings - Fork 55
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
FEATURE : support nested prefix #506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 prefix를 flush 하였을 때 해당 prefix의 child prefix들의
캐시 아이템들에 대해서도 flush 하는 코드가 존재하지 않습니다.
해당 부분도 구현해야합니다.
제가 잘못봤네요. 이미 구현되어 있네요. 코드 리뷰는 추가로 좀 더 진행하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 nested prefix 구조에서 첫번째 prefix만 mc_isvalidname 검사를 진행하고 하위 prefix에 대해서는 안하고 있습니다. prefix 모두에 대해서 검사하도록 수정해야할 것으로 보입니다.
또, 테스트 코드가 안 올라와있습니다. 올려주시고, 현재 테스트코드명이 issue_486.t 으로 되어있는데
nested_prefix.t로 바꾸는 게 좋겠습니다.
1차 리뷰 내용 반영하였습니다. 추가적으로, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저번 리뷰에서도 말씀드렸지만, 테스트 코드가 안 올라와있습니다. 올려주세요.
2차 리뷰 내용 반영하였습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3차 리뷰 내용 반영하였습니다.
@HarryKim93 |
네 알겠습니다. 정리된 이슈 내용 추후에 반영하도록 하겠습니다. |
@minkikim89 개인적인 생각으로는, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 여기까지 리뷰합니다.
한번 더 보고 리뷰 남기겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 NEW_PREFIX_STATS_MANAGEMENT로 disable되어있는 코드들도
구현되어져야 할 것 같습니다. stats_prefix_insert에서 nested prefix가
고려되어있지 않네요.
또, 현재 전체적으로 코드 스타일이 맞지 않는 부분들이 있어서 그 부분들도 한번
검토해서 맞춰주시면 좋겠습니다.
aeeb487
to
90272eb
Compare
@minkikim89 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 테스트 코드를 제외하고는 코드리뷰 마쳤습니다.
@jhpark816
현재 nested_prefix에서 prefix의 stats를 지우려하면
모든 prefix_stats를 스캔해야하는 상태인데요.
prefix가 많이 생기지 않을 것으로 예상하고 있고,
prefix를 delete 해야하는 상황도 많지 않을 것 같아서
현재대로 구현되어있어도 괜찮을 것 같은데 어떻게 생각하시나요?
@minkikim89 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HarryKim93
수고 많으셨습니다. 1차 리뷰하였습니다.
리뷰 사항 중 중요 논의 사항이 있으므로, 확인해 주시면 고맙겠습니다.
@HarryKim93 의견 감사합니다.
그리고, 마지막 의견으로, 모든 수정을 하나의 PR로 받기에는 코드 수정 및 리뷰 단위가 큰 것 같습니다.
|
352b9eb
to
ba9dbe4
Compare
7차 리뷰 반영했습니다. @jhpark816 |
@HarryKim93 검토하고 나서, 의견 드리겠습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현 상태에서도 review 할 양이 많네요.
일단 review 하였는 데, 검토 바랍니다.
ba9dbe4
to
b1e099b
Compare
리뷰 반영 했습니다. |
b1e099b
to
87c3d52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review 하였습니다. 확인 바랍니다.
@jhpark816
와 같이 에러가 뜨는 데, 해결 방법이 있을까요? |
86331f8
to
19b4f01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review 완료
binary.t 실패는 이번 review 수정 후에 다시 한번 보시죠. |
@jhpark816 |
bd67c1e
to
a801d53
Compare
@HarryKim93 수고 많으셨어요. |
a801d53
to
2cd61cd
Compare
2cd61cd
to
89b6b8f
Compare
@jhpark816 추가로, 이전에 언급하셨던 개별 prefix 조회에 관한 issue 올려주시면, skiplist 구현에 따른 논의 진행 후, 개발하겠습니다. |
Nested prefix 기능 지원을 위한 PR입니다.
types.h에 NESTED_PREFIX 추가하여, 수정한 부분은 태그로 처리했습니다.
기능 검증을 위해, issue_486.t 파일을 추가하였습니다.
해당 파일에서는 prefix 3개 child prefix 4개를 각각 만들어. prefix 3개를 flush하면 child prefix 또한 사라지는지 확인하였습니다.
-> 총 12개 prefix 생성. prefix 3개만 flush 시, 모든 child prefix들도 flush되는지 확인.
prefix.c
"stats prefixes" 수행 시, prefix 개수만 나오도록 추가했습니다.
그리고 prefix_link(), prefix_unlink(), perfix_bytes_infr(), prefix_bytes_decr(), prefix_isvalid()부분의 주석을 해제하였습니다.
stats.c
stats_prefix_find()에서 nested prefix로 key값을 주었을 때에, 기존 코드로 실행 시에는,
if(!mc_isvalidname(key, length))로 실행이 넘어가 적절한 실행이 되지 않았지만,
if(length > 0) 부분을 if (length > 0 && prefix_depth == 1) 부분으로 바꾸어 nested prefix로 key값을 주었을 때에도 정상동작하도록 수정하였습니다.
@minkikim89
확인 요청 드립니다.