Skip to content
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

INTERNAL: Modify the term related to eflag #332

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

ing-eoking
Copy link
Collaborator

🔗 Related Issue

⌨️ What I did

  • eflag 관련 용어를 변경합니다.
    • fwhere → offset
    • operand → bitwise.value
    • fvalue → value

Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

일부 리뷰

const size_t hexa_str_length= MEMCACHED_COLL_MAX_BYTE_STRING_LENGTH;
char foperand_str[hexa_str_length];
const size_t bitwvalue_length= MEMCACHED_COLL_MAX_BYTE_STRING_LENGTH;
char bitwvalue[bitwvalue_length];
Copy link
Contributor

Choose a reason for hiding this comment

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

bitwvalue_length와 bitwvalue 용어는 잘 맞지 않음.

hexa_str_length는 그대로 사용하고, foperand_str 대신에 value_str 이라고 할까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

value_str 로 변경하겠습니다.

{
return MEMCACHED_INVALID_ARGUMENTS;
}

ptr->options.is_initialized = true;
ptr->options.is_bitwised = false;

ptr->fwhere = fwhere;
ptr->flength = fvalue_length;
Copy link
Contributor

Choose a reason for hiding this comment

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

기존에 0보다 작거나 같은 fvalue_length을 주는 경우가 있었나요?
현재 코드를 보면, 그런 경우를 가정하는 것으로 보여 질문합니다.

Copy link
Collaborator Author

@ing-eoking ing-eoking Dec 24, 2024

Choose a reason for hiding this comment

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

memcached_coll_eflag_filter_init는 사용자가 직접적으로 사용하는 API라서 넣은 것으로 보입니다.

fvalue_length의 타입은 size_t로 0보다 작은 값을 넣을 수 없습니다.

Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

일부 리뷰.
전체를 다시 확인해 주세요.

- fwhere: eflag에서 비교 연산을 취할 데이터의 시작 offset을 바이트 단위로 지정한다.
- fvalue: 비교 연산을 취할 데이터 값을 지정한다.
- offset: eflag에서 비교 연산을 취할 데이터의 시작 offset을 바이트 단위로 지정한다.
- value: 비교 연산을 취할 데이터 값을 지정한다.
Copy link
Contributor

Choose a reason for hiding this comment

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

value => value, value_length

- fvalues: 비교 연산을 취할 데이터 값을 array 형태로 지정한다.
- fvalue_count: 비교 연산을 취할 데이터 값들의 개수를 지정한다.
- offset: eflag에서 비교 연산을 취할 데이터의 시작 offset을 바이트 단위로 지정한다.
- values: 비교 연산을 취할 데이터 값을 array 형태로 지정한다.
Copy link
Contributor

Choose a reason for hiding this comment

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

values => values, value_length

그리고, 아래 설명도 추가

모든 값의 길이는 동일하게 value_length이어야 한다.

const unsigned char *foperand,
const size_t foperand_length)
const unsigned char *bitwise_value,
const size_t bitwise_value_length,
Copy link
Contributor

Choose a reason for hiding this comment

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

  • value, value_length 용어를 사용하면 되지 않는지 ?

```

- bitwise_op: bitwise 연산을 지정한다.
- MEMCACHED_COLL_BITWISE_AND
- MEMCACHED_COLL_BITWISE_OR
- MEMCACHED_COLL_BITWISE_XOR
- foperand: eflag에서 bitwise 연산을 취할 operand를 지정한다.
- bitwise_value: eflag에서 bitwise 연산을 취할 operand를 지정한다.
Copy link
Contributor

Choose a reason for hiding this comment

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

인자 순서 변경

} bitwise;

struct {
memcached_coll_comp_t op;
memcached_hexadecimal_st fvalue[MEMCACHED_COLL_MAX_EFLAGS_COUNT];
memcached_hexadecimal_st values[MEMCACHED_COLL_MAX_EFLAGS_COUNT];
Copy link
Contributor

Choose a reason for hiding this comment

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

eflag_filter에 관한 함수들에서도 변경이 있어야 합니다.

@ing-eoking ing-eoking force-pushed the filter branch 3 times, most recently from ca33191 to 9f4387d Compare December 31, 2024 02:41
Copy link
Contributor

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

@@ -426,7 +426,7 @@ Response code는 아래와 같다.
- MEMCACHED_NOTHING_TO_UPDATE: 변경사항이 명시되지 않음.
- MEMCACHED_TYPE_MISMATCH: 주어진 key에 해당하는 자료구조가 B+tree가 아님.
- MEMCACHED_BKEY_MISMATCH: 주어진 bkey 유형과 해당 B+tree의 bkey 유형이 다름.
- MEMCACHED_EFLAG_MISMATCH: update_filter에 명시된 eflag 데이터가 존재하지 않음.
- MEMCACHED_EFLAG_MISMATCH: eflag_update 명시된 eflag 데이터가 존재하지 않음.
Copy link
Contributor

Choose a reason for hiding this comment

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

eflag_update => eflag_update에

@@ -4936,83 +4934,82 @@ memcached_return_t memcached_coll_eflag_filter_set_bitwise(memcached_coll_eflag_
return MEMCACHED_INVALID_ARGUMENTS;
}

if (ptr->flength != foperand_length)
if (ptr->comp.values->length != bitwise_value_length)
Copy link
Contributor

Choose a reason for hiding this comment

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

ptr->comp.values[0].length 로 표현하는 것이 좋겠습니다.

ptr->comp.fvalue.length = fvalue_length;
ptr->flength = fvalue_length;
ptr->value.array = (unsigned char *)value;
ptr->value.length = value_length;
Copy link
Contributor

Choose a reason for hiding this comment

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

아래 문장도 추가하시죠.

ptr->value.options.array_is_allocated = false;

const unsigned char *foperand,
const size_t foperand_length,
const unsigned char *bitwise_value,
const size_t bitwise_value_length,
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 value, value_length로 통일

@jhpark816
Copy link
Contributor

@ing-eoking
이전 리뷰 사항은 모두 수정된 상태인가요?

@jhpark816 jhpark816 merged commit 20f05e5 into naver:develop Dec 31, 2024
1 check passed
@ing-eoking ing-eoking deleted the filter branch January 2, 2025 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants