FEATURE : support nested prefix

[matteblack9]

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
확인 요청 드립니다.

[minkikim89]

현재 prefix를 flush 하였을 때 해당 prefix의 child prefix들의
캐시 아이템들에 대해서도 flush 하는 코드가 존재하지 않습니다.
해당 부분도 구현해야합니다.

제가 잘못봤네요. 이미 구현되어 있네요. 코드 리뷰는 추가로 좀 더 진행하겠습니다.

[minkikim89]

현재 nested prefix 구조에서 첫번째 prefix만 mc_isvalidname 검사를 진행하고 하위 prefix에 대해서는 안하고 있습니다. prefix 모두에 대해서 검사하도록 수정해야할 것으로 보입니다.
또, 테스트 코드가 안 올라와있습니다. 올려주시고, 현재 테스트코드명이 issue_486.t 으로 되어있는데
nested_prefix.t로 바꾸는 게 좋겠습니다.

[matteblack9]

1차 리뷰 내용 반영하였습니다.

추가적으로,
prefix.c의 prefix_link()에서 set 명령어 시, nested prefix에 대해서 mc_isvalidname()을 수행하는 부분이 없어 추가했습니다. 테스트 파일에는 이를 확인하는 테스트 코드를 추가했습니다.

[minkikim89]

저번 리뷰에서도 말씀드렸지만, 테스트 코드가 안 올라와있습니다. 올려주세요.

[matteblack9]

2차 리뷰 내용 반영하였습니다.
테스트 파일도 추가하였습니다.

[matteblack9]

3차 리뷰 내용 반영하였습니다.

[minkikim89]

@HarryKim93
#486 참조하셔서
추가적으로 구현해야할 부분 구현해주시면 다시 리뷰 진행하겠습니다.

[matteblack9]

@HarryKim93
#486 참조하셔서
추가적으로 구현해야할 부분 구현해주시면 다시 리뷰 진행하겠습니다.

네 알겠습니다. 정리된 이슈 내용 추후에 반영하도록 하겠습니다.

[matteblack9]

@minkikim89
4차 리뷰 내용 반영하였습니다. 몇달간 사정이 생겨 내용을 늦게 반영하게 되었습니다.
정리된 이슈 내용을 한번에 반영했습니다.

개인적인 생각으로는,
stats prefixes --all
stats detail dump --all
과 같은 옵션을 추가해 child prefix 들도 통계 가능한 기능을 개발하는 것도 추후 고려해보아야 될 것 같습니다.

[minkikim89]

일단 여기까지 리뷰합니다.
한번 더 보고 리뷰 남기겠습니다.

[minkikim89]

현재 NEW_PREFIX_STATS_MANAGEMENT로 disable되어있는 코드들도
구현되어져야 할 것 같습니다. stats_prefix_insert에서 nested prefix가
고려되어있지 않네요.
또, 현재 전체적으로 코드 스타일이 맞지 않는 부분들이 있어서 그 부분들도 한번
검토해서 맞춰주시면 좋겠습니다.

[matteblack9]

@minkikim89
5차 리뷰 반영하였습니다

[minkikim89]

우선 테스트 코드를 제외하고는 코드리뷰 마쳤습니다.

@jhpark816
현재 nested_prefix에서 prefix의 stats를 지우려하면
모든 prefix_stats를 스캔해야하는 상태인데요.
prefix가 많이 생기지 않을 것으로 예상하고 있고,
prefix를 delete 해야하는 상황도 많지 않을 것 같아서
현재대로 구현되어있어도 괜찮을 것 같은데 어떻게 생각하시나요?

[matteblack9]

@minkikim89
6차 리뷰 반영했습니다.

[jhpark816]

@HarryKim93
수고 많으셨습니다. 1차 리뷰하였습니다.
리뷰 사항 중 중요 논의 사항이 있으므로, 확인해 주시면 고맙겠습니다.

[jhpark816]

@HarryKim93

의견 감사합니다.

• backward compatibility 이슈
  • 실 서비스에 사용되는 arcus 모니터링 도구와 관련이 있는 문제라, 아마 예상하기 힘들었을 것입니다.
  • 기존에 최상위 prefix 통계만 수집하여 모니터링하고 있습니다.
  • 하위 prefix 까지 수집하게 되면, 보여주는 prefix 정보도 달라지게 되고, 수집 대상이 늘어 성능 이슈 가능성도 있습니다.
  • nested prefix를 지원한다면, 그에 맞게 수집 및 모니터링 뷰도 향후 재정리하여야 합니다.
• skip list이면 child 수에 제약을 두지 않아도 될 것 같습니다.
  • children 수가 많더라도, 특정 하나의 child 탐색 비용이 작기 때문입니다.
  • 따라서, child 수에 제약을 두지 않고, skip list로 구현하도록 하시죠.
  • skip list 구현에 있어서 child 수 제약이 필요하다면, 그 때 알려주시면 논의하여 결정하시죠.

그리고, 마지막 의견으로, 모든 수정을 하나의 PR로 받기에는 코드 수정 및 리뷰 단위가 큰 것 같습니다.
그래서, PR 단위를 아래와 같이 작은 단위로 나누는 것이 어떤가 싶습니다. 이에 대해 의견 바랍니다.

• 현재 PR은 nested prefix 지원을 위한 통계 정보 확장과 그에 따른 변경 방식의 코드로 한정하고요.
  대신, MAX_PREFIX_DEPTH는 기존처럼 1로 해야 할 것 같습니다.
• child prefix 정보들의 관리하는 skip list를 구현하고 MAX_PREFIX_DEPTH를 2로 변경하는 수정은
  next PR로 받습니다.

[matteblack9]

7차 리뷰 반영했습니다.

@jhpark816
개발을 하다보니, 불필요한 계산을 줄이기 위해
inclusive 값을 leaf prefix가 아닌 경우만 통계 정보를 가지는 것에 대한 처리가 꽤나 많아집니다.
leaf prefix인 경우에도 inclusive 값을 가지도록 일괄적으로 하면 구현이 간단해지긴 하는데, 어떤 방법이 좋을까요?

[jhpark816]

@HarryKim93 검토하고 나서, 의견 드리겠습니다.

[jhpark816]

현 상태에서도 review 할 양이 많네요.
일단 review 하였는 데, 검토 바랍니다.

[matteblack9]

@jhpark816

리뷰 반영 했습니다.

[jhpark816]

review 하였습니다. 확인 바랍니다.

[matteblack9]

@jhpark816
9차 리뷰 완료했습니다.
수정 후, binary.t 테스트를 여러번 반복 시, 통과할 때가 있고, 못할 때가 생깁니다.

#   Failed test 'Flags is set properly'
#   at ./t/binary.t line 72.
#          got: undef
#     expected: '19'
Argument "somevalue" isn't numeric in numeric eq (==) at ./t/binary.t line 73.
Argument "" isn't numeric in numeric eq (==) at ./t/binary.t line 73.
send: Cannot determine peer address at ./t/binary.t line 512.

와 같이 에러가 뜨는 데, 해결 방법이 있을까요?

[jhpark816]

review 완료

[jhpark816]

binary.t 실패는 이번 review 수정 후에 다시 한번 보시죠.

[matteblack9]

@jhpark816
10차 리뷰 완료했습니다.
binary.t 실패는 memchr에서 세번째 인자가 음수가 들어가는 문제 때문에 생긴 것 같아, nprefix 검사를 하는 것으로 해결 했습니다.

[jhpark816]

@HarryKim93 수고 많으셨어요.
1개 commit으로 만들어 주시고, rebase 필요한 지도 확인 바랍니다.

[matteblack9]

@jhpark816
리뷰 정말 감사했습니다.
1개 commit으로 만들었습니다. rebase는 필요하지 않아 보이는데, 추가적으로 점검할 것이 있나요?

추가로, 이전에 언급하셨던 개별 prefix 조회에 관한 issue 올려주시면, skiplist 구현에 따른 논의 진행 후, 개발하겠습니다.