INTERNAL: Add a swallow handler for mop by ing-eoking · Pull Request #835 · naver/arcus-memcached

ing-eoking · 2025-04-25T07:51:12Z

🔗 Related Issue

jam2in/arcus-works#710

⌨️ What I did

mop 명령어 중 swallow 처리가 필요한 명령어에 swallow 핸들링을 추가했습니다.
- mop get
- mop delete
mget, bop mget, bop smget 등에는 이미 swallow 로직이 적용되어 있었으나, mop 명령어에는 빠져 있어 이를 보완했습니다.

jhpark816 · 2025-04-29T04:28:29Z

@ing-eoking
본 PR도 완료해 주세요.

ing-eoking · 2025-04-29T04:35:52Z

@jhpark816

해당 PR도 작업은 완료되었습니다.
리뷰 진행주시면 될 것 같습니다.

jhpark816 · 2025-04-29T04:37:21Z

@ing-eoking 리뷰어 지정하세요.

namsic · 2025-06-09T06:16:59Z

+        if (((lenfields == 0 && numfields > 0) || (lenfields > 0 && numfields == 0)) ||
+            lenfields > ((numfields*MAX_FIELD_LENG) + numfields-1) ||
+            numfields > settings.max_map_size ||
+            numfields > ((lenfields/2) + 1)) {


기존 조건문을 하나로 합쳤는데, 아래와 같은 조건은 numfields가 0일 때는 의미상 맞지 않아 보입니다.
lenfields > ((numfields*MAX_FIELD_LENG) + numfields-1)

동작 여부와 별개로 lenfields와 numfields가 0일 때의 비교문으로는 일부 어색한 부분이 있는데,
코드 간결성 측면에서 더 낫다고 보는 것인가요?

lenfields 또는 numfields가 0인 경우는 이미 앞선 조건문
(lenfields == 0 && numfields > 0) || (lenfields > 0 && numfields == 0)에 의해 걸러지므로,
조건상 제외된다고 볼 수 있습니다.

따라서 코드 간결성 측면에서도 해당 조건들을 별도로 분리하기보다는 하나로 합치는 것이 더 좋을 것 같다고 생각됩니다.

jhpark816 · 2025-06-11T12:17:55Z

@ing-eoking

Related Issue는 관련이 없는 이슈로 보입니다. 확인해 주세요.
mop get/delete 처리 중 어떤 경우에 swallow 처리가 빠져서, 추가한 것 인지를 설명해 주세요.

ing-eoking · 2025-06-13T02:11:35Z

@jhpark816

Related Issue는 관련이 없는 이슈로 보입니다. 확인해 주세요.

변경되었습니다.

mop get/delete 처리 중 어떤 경우에 swallow 처리가 빠져서, 추가한 것 인지를 설명해 주세요.

두 명령어(mop get, mop delete)모두 해당됩니다.

mop 명령어 중 swallow 처리가 필요한 명령어에 swallow 핸들링을 추가했습니다.
  - mop get
  - mop delete

jhpark816 · 2025-06-13T03:29:50Z

mop get/delete 처리 로직에는 swallow 기능이 들어가 있는 것으로 알고 있습니다.
처리 과정 중, 어떤 경우에 대해 swallow 처리가 빠져서, 추가한 것 인지를 설명해 주세요.

ing-eoking · 2025-06-13T07:32:16Z

@jhpark816

mop get/delete 처리 로직에는 swallow 기능이 들어가 있는 것으로 알고 있습니다.
처리 과정 중, 어떤 경우에 대해 swallow 처리가 빠져서, 추가한 것 인지를 설명해 주세요.

lenfields와 numfields 값이 memcached에서 허용되지 않는 경우에 해당합니다. 구체적으로는 다음 조건 중 하나라도 만족하는 경우입니다.

lenfields와 numfields 중 한쪽만 0인 경우
lenfields 값이 허용 가능한 최대 크기를 초과한 경우
numfields 값이 max_map_size를 초과한 경우
numfields 값이 lenfields / 2 + 1보다 큰 경우

jhpark816 · 2025-06-13T07:39:09Z

@ing-eoking
제시한 경우에는 numfields와 lenfields 자체가 valid 하지 않은 상태입니다.
이와 같은 invalid 정보를 바탕으로 swallow 작업은 수정하지 않도록 한 것이 기존 코드의 의도입니다.
Swallow 수행하지 않는 대신 CLIENT_ERROR 리턴하여 client에서 reconnect 처리하게 한 것입니다.
따라서, 수정이 필요 없을 것 같습니다.

ing-eoking · 2025-06-13T07:53:24Z

@jhpark816

여러 개의 키를 받기 위해 nread를 수행하는 경우, 다른 명령어들(아래의 명령어)에서는
lenfields와 numfields 중 한쪽만 0인 경우를 제외하고 위 조건에 해당할 경우 swallow 상태로 넘어가도록 되어 있습니다.

mget
bop mget / bop smget

혹시 mop 명령어에서만 이런 방식으로 처리하는 이유가 있을까요?

jhpark816 · 2025-06-13T07:58:44Z

@ing-eoking @namsic
어느 쪽이 맞는 구현인 지를 확인해 주세요.

ing-eoking · 2025-06-13T08:20:17Z

@jhpark816

max_map_size나 허용 가능한 최대 크기 등의 값은 추후에 memcached 버전 변화에 따라 달라질 수 있습니다.
그렇기 때문에 이 때마다 reconnect를 하는 것은 적절하지 않다고 생각됩니다.

따라서 아래와 같은 기준으로 예외 상황을 핸들링하는 방식이 적절하다고 판단됩니다.

lenfields 또는 numfields 중 하나만 0일 경우 → reconnect
lenfields가 허용 가능한 최대 크기를 초과하는 경우 → swallow
numfields가 max_map_size를 초과하는 경우 → swallow
numfields가 lenfields / 2 + 1보다 클 경우 → reconnect

jhpark816 · 2025-06-13T09:22:15Z

@ing-eoking
아래 명령에서 어떤 경우에 swallow 하나요?

mget
bop mget / bop smget

jhpark816 · 2025-06-13T09:26:21Z

@ing-eoking

lenfields가 허용 가능한 최대 크기를 초과하는 경우 → swallow
- lenfields가 정상적인 값이 아니라 어떤 버그로 잘못된 값을 가졌을 가능성이 있습니다.
- 잘못된 값을 기준으로 swallow 작업은 오히려 더 문제를 일으킬 수 있습니다.
- 따라서, 조금이라도 잘못된 값일 가능성이 있다면 CLIENT_ERROR 처리해야 합니다.
numfields가 max_map_size를 초과하는 경우 → swallow
- 이 경우도, 응용에서 max_map_size를 몰라서 큰 값을 주었을 수도 있지만
  어떤 버그도 인해 잘못된 값일 들어 갔을 가능성도 있습니다.
- 따라서, CLIENT_ERROR 처리하는 것이 나은 것 같습니다.

ing-eoking · 2025-06-16T00:20:11Z

@jhpark816

lenfields가 정상적인 값이 아니라 어떤 버그로 잘못된 값을 가졌을 가능성이 있습니다.

해당 케이스는 set key 0 0 1을 시도했지만 버그로 인해
실제로는 set i 0 0 10000000처럼 value 크기가 1MB를 초과하는 요청이 되는 경우와 유사하다고 생각됩니다.
이 경우 서버에서는 CLIENT_ERROR object too large for cache를 반환하고 swallow 처리를 하지만,
이와 같은 상황까지 서버에서 완전히 대응하기는 어렵다고 생각됩니다.

이와 같은 invalid 정보를 바탕으로 swallow 작업은 수정하지 않도록 한 것이 기존 코드의 의도입니다.

mop 명령어에서 swallow 처리를 하지 않는 것이 의도된 동작이라면, 추후 swallow 처리를 어떻게 할지 논의할 수 있도록 이슈를 생성해두는 것이 좋을 것 같습니다. 현재로서는 swallow 처리가 필요한 경우에도 응답 메시지를 INVALID로 변경하지 않고, 기존대로 CLIENT_ERROR를 유지하여 클라이언트가 reconnect 하도록 하는 방향이 적절해 보입니다.

jhpark816 · 2025-06-16T03:57:49Z

아래 명령에서는 어떤 경우에 swallow 하나요? mop 연산 과는 어떤 차이가 있나요?

mget
bop mget / bop smget

ing-eoking · 2025-06-16T05:17:59Z

@jhpark816

아래 명령에서는 어떤 경우에 swallow 하나요? mop 연산 과는 어떤 차이가 있나요?

mget

numkeys 값이 허용된 범위를 초과한 경우
lenkeys 값이 최대 허용 크기를 초과한 경우
numkeys 값이 lenkeys / 2 + 1보다 큰 경우

bop mget / bop smget

numkeys 값이 허용된 범위를 초과한 경우
lenkeys 값이 최대 허용 크기를 초과한 경우
numkeys 값이 lenkeys / 2 + 1보다 큰 경우
count 값이 0이거나 count 또는 offset + count 값이 제한을 초과하는 경우

이러한 조건들은 대부분 mop 연산의 조건과 유사합니다.
mop 연산에서는 lenfields와 numfields 중 한쪽만 0인 조건이 추가적으로 들어가 있으며,
bop mget / bop smget 연산에서는 count 또는 offset + count 값에 따라 swallow가 발생할 수 있습니다.

jhpark816

리뷰 완료

jhpark816 · 2025-06-16T06:10:54Z

-                out_string(c, "CLIENT_ERROR bad value");
-                return;
-            }
+        if (((lenfields == 0 && numfields > 0) || (lenfields > 0 && numfields == 0)) ||


lenfields가 0이거나 numfields가 0인 경우는 기존대로 합시다.
mget에서도 그렇게 되어 있습니다.

수정되었습니다.

jhpark816

리뷰 완료

jhpark816 · 2025-06-16T07:06:48Z

-            (! safe_strtoul(tokens[MOP_KEY_TOKEN+2].value, &numfields))) {
+            (! safe_strtoul(tokens[MOP_KEY_TOKEN+2].value, &numfields)) ||
+            (lenfields > (UINT_MAX-2)) || (lenfields == 0 && numfields > 0) ||
+            (lenfields > 0 && numfields == 0)) {


아래 조건이면 될 것 같습니다.

(lenkeys > (UINT_MAX-2)) || (lenkeys == 0) || (numkeys == 0)) {

이 경우, map 자료구조에서 전체 조회나 전체 삭제 기능이 제공되지 않게 되는데,
문서에서 전체 조회나 전체 삭제에 대한 언급은 따로 없으니 제거하는 편이 좋을까요?

ing-eoking marked this pull request as draft April 25, 2025 07:57

ing-eoking force-pushed the swallow branch 3 times, most recently from 899fe0e to ae3cfda Compare April 25, 2025 08:12

ing-eoking marked this pull request as ready for review April 29, 2025 04:35

ing-eoking requested a review from namsic April 29, 2025 04:49

namsic reviewed Jun 9, 2025

View reviewed changes

ing-eoking force-pushed the swallow branch from ae3cfda to c5fa19a Compare June 13, 2025 02:30

jhpark816 reviewed Jun 16, 2025

View reviewed changes

ing-eoking marked this pull request as draft June 16, 2025 06:49

INTERNAL: Add a swallow handler for mop

7a236b2

ing-eoking force-pushed the swallow branch from 76dfe0c to 7a236b2 Compare June 16, 2025 06:53

ing-eoking marked this pull request as ready for review June 16, 2025 06:55

jhpark816 reviewed Jun 16, 2025

View reviewed changes

jhpark816 approved these changes Jun 16, 2025

View reviewed changes

jhpark816 merged commit 2626bcb into naver:develop Jun 16, 2025
1 check passed

ing-eoking deleted the swallow branch June 16, 2025 07:28

ing-eoking mentioned this pull request Jun 20, 2025

INTERNAL: Refactor the len/numfields validation logic #839

Merged

Conversation

ing-eoking commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Related Issue

⌨️ What I did

Uh oh!

jhpark816 commented Apr 29, 2025

Uh oh!

ing-eoking commented Apr 29, 2025

Uh oh!

jhpark816 commented Apr 29, 2025

Uh oh!

namsic Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

ing-eoking Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

jhpark816 commented Jun 11, 2025

Uh oh!

ing-eoking commented Jun 13, 2025

Uh oh!

jhpark816 commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ing-eoking commented Jun 13, 2025

Uh oh!

jhpark816 commented Jun 13, 2025

Uh oh!

ing-eoking commented Jun 13, 2025

Uh oh!

jhpark816 commented Jun 13, 2025

Uh oh!

ing-eoking commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhpark816 commented Jun 13, 2025

Uh oh!

jhpark816 commented Jun 13, 2025

Uh oh!

ing-eoking commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhpark816 commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ing-eoking commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mget

bop mget / bop smget

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

jhpark816 Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

ing-eoking Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

jhpark816 Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

ing-eoking Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ing-eoking commented Apr 25, 2025 •

edited

Loading

jhpark816 commented Jun 13, 2025 •

edited

Loading

ing-eoking commented Jun 13, 2025 •

edited

Loading

ing-eoking commented Jun 16, 2025 •

edited

Loading

jhpark816 commented Jun 16, 2025 •

edited

Loading

ing-eoking commented Jun 16, 2025 •

edited

Loading

ing-eoking Jun 16, 2025 •

edited

Loading