INTERNAL: add a function to generate a bkey string #326

ing-eoking · 2024-12-12T03:20:31Z

🔗 Related Issue

jam2in/arcus-works#655

⌨️ What I did

bkey string을 생성하는 공통된 로직을 하나의 함수로 넣어 처리합니다.
- Invalid query type의 경우, -1을 리턴합니다.

libmemcached/collection.cc

jhpark816

일부 리뷰

jhpark816 · 2024-12-16T04:15:51Z

libmemcached/collection.cc

@@ -215,6 +215,59 @@ static inline bool space_separated_keys_is_supported(memcached_server_write_inst
  return false;
 }

+static int generate_str_bkey_from_query(memcached_coll_query_st *query,
+                                        char *buffer, const int buffer_length,
+                                        bool support_range, bool *is_descending)


is_descending 인자를 제거합시다.

smget 로직을 보니, result 객체를 설정하기 위한 용도인 데,
이 부분을 다루는 별도 함수를 두는 것이 나을 것 같습니다.

참고로, result 객체를 설정하는 코드를 먼저 리팩토링해야
현재의 PR을 반영할 수 있을 것 같습니다.

libmemcached/collection.cc

jhpark816

리뷰 완료

libmemcached/collection.cc

jhpark816 · 2024-12-26T06:41:30Z

@ing-eoking 리뷰해야 하는 상태가 되면, noti 코멘트를 남겨주세요.

jhpark816

일부 리뷰

jhpark816 · 2024-12-26T12:55:04Z

libmemcached/collection.cc

-    }
-    else if (MEMCACHED_COLL_QUERY_BOP_EXT == query->type)
+    write_length= generate_str_bkey_from_query(query, buffer, buffer_length, false, rc);
+    if (rc != MEMCACHED_SUCCESS)


아래와 같이 write_length 값으로 실패 여부를 판단할 수 있게 하면 좋겠습니다.
그리고, 어떤 오류든 설정해 두는 것도 괜찮을 것 같습니다.

if (write_length < 0) { return memcached_set_error(*ptr, rc, MEMCACHED_AT, memcached_literal_param("Invalid bkey query.\n")); }

참고로, 리턴 값이 rc 였다면, 아래 형태의 조건이 좋을 것입니다.

if (rc != MEMCACHED_SUCCESS)

jhpark816

리뷰 완료

jhpark816 · 2024-12-27T06:13:09Z

libmemcached/collection.cc

-                                bkey_str, MEMCACHED_COLL_MAX_BYTE_STRING_LENGTH);
-
-      write_length= snprintf(buffer, buffer_length, " 0x%s", bkey_str);
+      if (rc == MEMCACHED_INVALID_ARGUMENTS)


MEMCACHED_INVALID_ARGUMENTS 오류만 로그를 남길 필요가 있나요?
둘 다 남기거나 둘 다 남기지 않는 것이 좋겠습니다.

jhpark816

추가 리뷰

libmemcached/collection.cc

jhpark816 · 2024-12-27T08:17:29Z

위의 PR (#331) 이 merge된 상태이므로,
최신 code 기준으로 rebase 해 주세요.

namsic · 2024-12-27T09:51:38Z

libmemcached/collection.cc

@@ -215,6 +215,60 @@ static inline bool space_separated_keys_is_supported(memcached_server_write_inst
  return false;
 }

+static int generate_str_bkey_from_query(memcached_coll_query_st *query,
+                                        char *buffer, const int buffer_length,
+                                        bool support_range, memcached_return_t &rc)


rc pointer type 사용하면 좋겠습니다.

namsic · 2024-12-30T00:34:21Z

libmemcached/collection.cc

+                                        char *buffer, const int buffer_length,
+                                        bool support_range, memcached_return_t *rc)
+{
+  int write_length= -1;


에러 발생하지 않았을 때도 rc 값을 설정해 주는 것이 맞아 보입니다.

generate_str_bkey_from_query는 내부 함수이며, rc 값이 이미 특정값으로 설정되었음을 가정합니다.
기존에 설정된 rc 값을 다시 설정할 필요는 없을 것 같습니다.

이미 설정된 rc 특정값은 무엇인가요?

에러가 발생하지 않았는데 기 설정된 rc값을 덮어쓰게 되면 문제가 있고,
에러가 발생했을 때는 기존 rc값을 무시하고 새로운 값으로 변경해도 문제가 없는 것인가요?

rc 값을 설정하는 것에는 문제가 없지만, generate_str_bkey_from_query 함수는 가장 초기에 호출되는 함수가 아니므로, 이 함수가 호출될 때 rc 값은 이미 초기화되어 있다고 가정하고 있습니다.
에러 발생 여부는 리턴값이 -1인지를 통해 확인할 수 있으며, rc 값은 에러의 더 구체적인 원인을 제공하는 용도로 사용됩니다.
따라서, 에러가 발생하지 않은 상황에서 rc 값을 다시 설정할 필요는 없다고 생각합니다.

만약 write_length와 rc 두 값을 generate_str_bkey_from_query 함수의 리턴값으로 처리한다고 본다면, 오히려 rc를 리턴하고 write_length를 포인터 인자로 받는 방식이 더 깔끔했을 것입니다.

rc라는 용어는 혼동을 일으킬 수 있으므로, 이를 error라는 용어로 변경하는 편이 좋을 것 같습니다.

jhpark816 requested a review from uhm0311 December 12, 2024 03:22

ing-eoking marked this pull request as draft December 12, 2024 05:13

ing-eoking force-pushed the bkey branch 2 times, most recently from 82010a6 to 8a2737c Compare December 12, 2024 06:01

jhpark816 requested review from namsic and removed request for uhm0311 December 12, 2024 06:35

ing-eoking force-pushed the bkey branch 2 times, most recently from 562cf42 to efe58d1 Compare December 12, 2024 08:00

ing-eoking marked this pull request as ready for review December 12, 2024 08:02

namsic reviewed Dec 13, 2024

View reviewed changes

libmemcached/collection.cc Show resolved Hide resolved

libmemcached/collection.cc Outdated Show resolved Hide resolved

libmemcached/collection.cc Outdated Show resolved Hide resolved

ing-eoking force-pushed the bkey branch 2 times, most recently from bd943db to 76b7318 Compare December 16, 2024 01:26

namsic approved these changes Dec 16, 2024

View reviewed changes

jhpark816 reviewed Dec 16, 2024

View reviewed changes

ing-eoking marked this pull request as draft December 16, 2024 07:05

jhpark816 reviewed Dec 17, 2024

View reviewed changes

libmemcached/collection.cc Show resolved Hide resolved

ing-eoking mentioned this pull request Dec 24, 2024

INTERNAL: Add a function to initialize a smget_result struct #331

Merged

ing-eoking force-pushed the bkey branch from 76b7318 to b28034a Compare December 26, 2024 05:37

ing-eoking marked this pull request as ready for review December 26, 2024 05:39

ing-eoking force-pushed the bkey branch from b28034a to 0dd5633 Compare December 26, 2024 09:00

jhpark816 reviewed Dec 26, 2024

View reviewed changes

ing-eoking force-pushed the bkey branch from 0dd5633 to 3d27474 Compare December 27, 2024 01:37

jhpark816 reviewed Dec 27, 2024

View reviewed changes

libmemcached/collection.cc Show resolved Hide resolved

ing-eoking force-pushed the bkey branch from 3d27474 to c878928 Compare December 27, 2024 07:42

ing-eoking force-pushed the bkey branch from c878928 to 199955b Compare December 27, 2024 09:00

jhpark816 approved these changes Dec 27, 2024

View reviewed changes

namsic reviewed Dec 27, 2024

View reviewed changes

ing-eoking force-pushed the bkey branch from 199955b to c3b8ad8 Compare December 30, 2024 00:11

namsic reviewed Dec 30, 2024

View reviewed changes

INTERNAL: Add a function to generate a bkey string

88dea87

ing-eoking force-pushed the bkey branch from c3b8ad8 to 88dea87 Compare December 30, 2024 03:20

jhpark816 approved these changes Dec 30, 2024

View reviewed changes

jhpark816 merged commit e205daa into naver:develop Dec 30, 2024
1 check passed

ing-eoking deleted the bkey branch December 30, 2024 07:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: add a function to generate a bkey string #326

INTERNAL: add a function to generate a bkey string #326

ing-eoking commented Dec 12, 2024

jhpark816 left a comment

jhpark816 Dec 16, 2024

jhpark816 left a comment

jhpark816 commented Dec 26, 2024 •

edited

Loading

jhpark816 left a comment

jhpark816 Dec 26, 2024

jhpark816 left a comment

jhpark816 Dec 27, 2024

jhpark816 left a comment

jhpark816 commented Dec 27, 2024 •

edited

Loading

namsic Dec 27, 2024

namsic Dec 30, 2024

ing-eoking Dec 30, 2024

namsic Dec 30, 2024

ing-eoking Dec 30, 2024

INTERNAL: add a function to generate a bkey string #326

INTERNAL: add a function to generate a bkey string #326

Conversation

ing-eoking commented Dec 12, 2024

🔗 Related Issue

⌨️ What I did

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

jhpark816 commented Dec 26, 2024 • edited Loading

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 left a comment

Choose a reason for hiding this comment

jhpark816 commented Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Dec 26, 2024 •

edited

Loading

jhpark816 commented Dec 27, 2024 •

edited

Loading