CLEANUP: Move event callback functions to mc_util files #713
Conversation
ing-eoking
force-pushed
the
develop-util
branch
2 times, most recently
from
97f89c3
to
b0c9f49
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
ing-eoking
force-pushed
the
develop-util
branch
from
b0c9f49
to
552840b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ing-eoking
changed the title
|
@namsic 리뷰 진행합시다. |
There was a problem hiding this comment.
@jhpark816
언뜻 보기에 callback function의 구현은 engine과 직접 관련이 없어 보이는데,
engine_xxx 형태의 변수 네이밍이나 ENGINE_HANDLE 타입을 인자로 받는 등의 이유가 있을까요?
mc_util.h
Outdated
| @@ -17,6 +17,7 @@ | |||
| #ifndef MC_UTIL_H | |||
| #define MC_UTIL_H | |||
|
|
|||
| #include <memcached/engine.h> | |||
There was a problem hiding this comment.
engine.h가 아니라 callback.h만 include해도 충분할 것으로 생각합니다.
There was a problem hiding this comment.
해당 내용 수정되었습니다.
별개로 다른 모듈에서의 헤더 계층 구조도 이상하다고 생각됩니다.
engine.h에서는 extension.h를 include하고 있기에 따로 include할 이유가 없습니다.
#include <memcached/extension.h>
#include <memcached/engine.h>위와 같이 모듈을 include하고 있는 모듈은 아래와 같습니다.
- extension_logger
- memcached
- syslog_logger
- item_base
- mock_server(test용)
이는 해당 모듈이 extension과 engine API를 활용한다는 것을 알려주는 목적인가요?
개인적으로 저는 extension에 있는 token_t가 왜 있는지도 모르겠습니다.
util이나 types 모듈에 넣는게 낫지 않을까? 생각됩니다.
There was a problem hiding this comment.
@ing-eoking
구현 의도는 모릅니다.
여튼, 본 이슈 후에 아래 2가지 사항을 이슈로 등록하여 처리하시죠.
- include 관계 정리
- extension.h 파일 정리
mc_util.c
Outdated
| /* event handlers structure */ | ||
| static struct engine_event_handler { | ||
| EVENT_CALLBACK cb; | ||
| const void *cb_data; | ||
| struct engine_event_handler *next; | ||
| } *engine_event_handlers[MAX_ENGINE_EVENT_TYPE + 1]; |
There was a problem hiding this comment.
개인적으로 기존처럼 구조체 정의와 배열 선언이 분리되어 있는 편이 더 읽기 좋다고 생각합니다.
There was a problem hiding this comment.
engine_event_handlers 배열이 초기화되는 것이 보장되나요?
There was a problem hiding this comment.
@namsic
구조체 내부 길이가 길지 않아 변수 선언과 구조체 내부가 함께 있으면,
내부 정보를 같이 볼 수 있어 코드에 대한 빠른 파악이 될 수 있다고 생각했습니다.
분리되어 있는 편이 읽기가 편하시다면 변경하겠습니다.
추가 의견으로 저는 별개로 분리 시 eigne_event_handler 구조체를 memcached.h와 같이
mc_util.h에 위치시켜 외부로 드러낼 필요는 없다고 생각됩니다.
@jhpark816
해당 구조체 변수는 global 변수이므로 OS에서 무조건 0으로 초기화가 보장됩니다.
There was a problem hiding this comment.
eigne_event_handler 구조체를 헤더 파일에 두지 맙시다.
There was a problem hiding this comment.
@namsic
의견 부탁드립니다.
There was a problem hiding this comment.
@ing-eoking
eigne_event_handler 구조체를 여기에 두더라도,
구조체 정의와 배열 선언을 분리하자는 의견입니다.
There was a problem hiding this comment.
@jhpark816
구조체 정의와 배열 선언을 분리할 경우
구조체 정의를 소스의 맨 위로 두거나
아래와 같이 구현되어야 합니다.
/* event handlers structure */
struct engine_event_handler {
EVENT_CALLBACK cb;
const void *cb_data;
struct engine_event_handler *next;
};
static struct engine_event_handler *engine_event_handlers[MAX_ENGINE_EVENT_TYPE + 1];예제의 형태는 크게 차이가 없다고 생각되어서
구조체 정의를 소스의 맨 위로 위치시키는게 읽기 편하신 형태인건지 궁금하여 요청드렸습니다.
t/topkeys.t
Outdated
| @@ -6,6 +6,8 @@ use FindBin qw($Bin); | |||
| use lib "$Bin/lib"; | |||
| use MemcachedTest; | |||
|
|
|||
| delete $ENV{"MEMCACHED_TOP_KEYS"}; | |||
There was a problem hiding this comment.
수정되었습니다.
테스트 수정은 크지 않아 branch를 변경안하고 push하다보니 잘못 들어갔습니다.
ing-eoking
force-pushed
the
develop-util
branch
2 times, most recently
from
db87318
to
105f0b7
Compare
|
@namsic 리뷰 바랍니다. |
|
메일이 왔었네요. 늦게 확인했습니다. |
기존 내용은 노션 및 이슈에 정리되었으므로 지웠습니다.
event callbacks 함수를
mc_util파일로 이동시킵니다.