Skip to content

INTERNAL: Fix a smget handling about trimmed result without bkey#286

Draft
ing-eoking wants to merge 1 commit into
naver:developfrom
ing-eoking:perr
Draft

INTERNAL: Fix a smget handling about trimmed result without bkey#286
ing-eoking wants to merge 1 commit into
naver:developfrom
ing-eoking:perr

Conversation

@ing-eoking

@ing-eoking ing-eoking commented Mar 21, 2024

Copy link
Copy Markdown
Collaborator

🔗 Related Issue

  • jam2in/arcus-works#480

⌨️ What I did

  • smget(new) 연산 시 VALUE 결과가 0인 result에 대해 Trimmed Key 비교 연산을 수행하지 않게 끔 변경했습니다.
  • 서버에서 받은 모든 bkey type에 대해 사용자가 입력한 bkey type과 다를 경우 PROTOCOL_ERROR를 반환하도록 변경했습니다.
    • (기존) Value에서 받은 element의 bkey type과 trimmed key에서의 bkey type이 다른 경우 PROTOCOL_ERROR를 반환
  • 일부 uninitialized variable error를 수정했습니다.

🤔 Others

  • 서버의 Response 메시지
    ELEMENTS 0
    MISSED_KEYS 0
    TRIMMED_KEYS 1
    b 201
    END
    
  • 변경된 반환 결과
    • 기존
    memcached smget(new) result : PROTOCOL ERROR, response : PROTOCOL ERROR
    
    • 변경
    memcached smget(new) result : SUCCESS, response : SERVER END
    

@ing-eoking ing-eoking marked this pull request as draft March 21, 2024 03:03
@ing-eoking ing-eoking self-assigned this Mar 21, 2024
@ing-eoking ing-eoking requested review from jhpark816 and namsic March 26, 2024 00:44
@ing-eoking ing-eoking marked this pull request as ready for review March 26, 2024 00:44
@jhpark816

Copy link
Copy Markdown
Contributor

@namsic 리뷰 바랍니다.

@namsic

namsic commented Oct 8, 2024

Copy link
Copy Markdown
Collaborator

@ing-eoking

최신 commit 기준으로 rebase 한 번 해주면 좋겠습니다.

  • smget(new) 연산 시 VALUE 결과가 0인 result에 대해 Trimmed Key 비교 연산을 수행하지 않게 끔 변경했습니다.
  • 서버에서 받은 모든 bkey type에 대해 사용자가 입력한 bkey type과 다를 경우 PROTOCOL_ERROR를 반환하도록 변경했습니다.

이렇게 변경해야 하는 이유에 대해 간단히 설명해 주면 좋겠습니다.

@ing-eoking

ing-eoking commented Oct 8, 2024

Copy link
Copy Markdown
Collaborator Author

@namsic

이렇게 변경해야 하는 이유에 대해 간단히 설명해 주면 좋겠습니다.

해당 PR이 조금 오래되어 자세히 기억하려면, 조금 시간이 걸릴 것 같습니다.

아래의 링크에서 간단히 설명되어 있습니다.
https://github.com/jam2in/arcus-works/issues/480#issuecomment-2006261497

smget(new) 연산 시 VALUE 결과가 0인 result에 대해 Trimmed Key 비교 연산을 수행하지 않게 끔 변경했습니다.
서버에서 받은 모든 bkey type에 대해 사용자가 입력한 bkey type과 다를 경우 PROTOCOL_ERROR를 반환하도록 변경했습니다.

eflag로 인해 모든 값이 skip되는 경우, element는 없지만 Trimmed key만 존재하게 됩니다. 서버에서 전송하는 Trimmed key와 bkey의 타입이 다를 가능성은 없다고 생각하여, 이를 제외하고 사용자가 지정한 bkey 타입과 서버에 저장된 bkey 타입과 비교할 수 있도록 변경한 것 같습니다.

@namsic namsic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 PR이 조금 오래되어 자세히 기억하려면, 조금 시간이 걸릴 것 같습니다.
아래의 링크에서 간단히 설명되어 있습니다.

이슈에 적힌 원인과 PR의 변경만으로는 잘 매칭이 되지 않아서 물었습니다.
문제가 생기는 원인과 그 문제를 어떻게 해결하려고 했던 것인지 조금 더 풀어서 설명해주면 좋겠습니다.

Comment thread libmemcached/fetch.cc
} else {
comp_result= (prev_sub_key->bkey == curr_sub_key->bkey) ? 0
: (prev_sub_key->bkey < curr_sub_key->bkey) ? -1 : 1;
if (merged->value_count > 0) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조건문을 아래와 같이 변경할 수 있나요?
value_count가 0이면, trimed_keys를 merge하지 않아야 하나요?

if (merged->value_count == 0) {
    should_merge = true;
} else if (merged->value_count < merged->count) {
    /*
     * compare sub_keys
     */

    if (comp_result <= 0) {
        should_merge = true;
    }
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value_count가 0이면, trimed_keys를 merge하지 않아야 하나요?

잘 모르겠네요. 좀 더 여러가지 상황을 고려해서 다시 짜야할 것 같습니다.
그러므로 잠시 해당 PR은 보류하겠습니다.

value_count == 0 인 상황에서 should_merge가 되어야 한다는 것은
Trimed key가 탐색한 탐색한 Btree 내 범위 중 최소여야 한다는 것이 보장되어야 합니다.

↓       Search Range        ↓   
----- |--Not Trimmed--| |--Trimed--|

But, all skipped by eflag!

@ing-eoking ing-eoking marked this pull request as draft October 10, 2024 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants