Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[CBRD-25436] Creating monitoring thread and data structures for cub_server monitoring #5293
[CBRD-25436] Creating monitoring thread and data structures for cub_server monitoring #5293
Changes from 15 commits
9216977
5fa32d1
53d83fd
1214fb9
e35c2a8
27144c4
29a41b4
dc02a08
6c04ca7
93476d6
bd0e96b
6973275
419557b
c507718
5d7dc21
33920c9
503ede2
72c60eb
96b0ebd
a0b20c4
d3c8708
d842e7c
e838efa
7e3f2df
4d0a6ca
9e47e76
f46fe82
a7be8ff
daee3e1
9cdbea3
5b9f8cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필요 없어 보이네요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제거하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
master_util_config_startup()에서 sysprm_load_and_init() 호출을 통해 시스템 파라미터 관련된 모듈이 초기화 되기 때문에 HA_DISABLED() 호출이 정상적으로 동작하지 않을 것 입니다. main 함수 아래 쪽에 HA_DISABLED() 조건 검사 후 hb_master_init()를 호출해 주는 코드부가 있습니다. 해당 위치로 옮겨주시면 좋겠네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위치 변경하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별도의 API 함수를 제공할 계획인가요? 단일 인스턴스를 통해 접근 시 필요해 보이진 않습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전 코드의 잔재입니다. 제거하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class server_entry;
불필요한 전방 선언이며, using server_entry_uptr_t를 server_entry class 정의 뒤 쪽으로 옮기면,class server_monitor_list;
역시 제거 가능There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_instance()가 호출되는 main 함수에서 생성자 호출되어 생성된 후 프로세스 종료 시 소멸자 호출되어 정리되겠네요. 추후 확장 시 할당된 instance 자체에 대한 정리는 쉽지 않을 것 같습니다. 아마도
static server_monitor *instance;
처럼 포인터 타입을 사용해야할 것 같은데요. 혹시 이에 대해 고려하신 부분이 있을까요?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server_monitor* server_monitoring_manager
를get_instance
함수를 통해 가져오고, 이후 확장 시 할당된 instance를 manual하게 해제해야 하는 상황에서는delete_instance
함수를 통해 지우면 될 것 같습니다. 해당 함수 추가하였습니다.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 클래스의 생성자/소멸자 다음에 위치시키는게 가독성에 도움이 될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT:
가독성을 위해 변수를 가장 마지막에 배치하고, 함수는 생성자 등에 이어서 배치하는게 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
각 클래스에 대해, 생성자-함수-변수 순으로 정의하도록 전체적으로 배치를 정리하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server_monitor_list를 별도의 내부 클래스로 유지하는 목적이 있을까요? server_entry를 위한 std::verctor<>는 server_monitor에 위치시켜도 충분할 것 같습니다. using을 이용해서 적당한 타입을 선언해 주면 될 것 같구요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후에
server_monitor_list
에 대한 탐색/삽입/제거/서버 revive 관련 함수들을 배치할 때,server_monitor
class에 배치시키기 보다는 list에 대한 함수로서 배치하는 것이 더 보기 좋아보인다고 생각해서 별도의 클래스로 설계하였습니다.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::vector 자체가 list의 의미를 담고 있습니다. 다음 경우가 아니라면 크게 의미는 없지 않을까요?
그렇지 않다면, server_monitor는 std::vector에 접근하기 위해 server_monitor_list에 한번 더 접근해야 하기에 코드 복잡도가 증가합니다. 또한, class 타입 증가, 부모 class 라인수 증가 등의 이슈도 있습니다.
이후 진행될 sub-task에서 server_monitor_list 클래스 사용의 이점이 설명될 수 있다면, 이 리뷰는 해당 이슈에서 추가 논의해도 좋습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server_monitor가 현재 위 두 경우에 해당하는 것 같지는 않습니다. 따라서 말씀 주신 대로, server_monior_list를 별개의 class로 설정하는 것 보다 server_monitor의 멤버로 직접 두는 것이 훨씬 간편할 것 같습니다.
해당 내용 반영하였고, 추가로 server_monitor_list 내부에 저장하고 있던 system parameter들도 사용 시 직접 가져와 사용하는 것이 클래스 구조를 간소화 하는데 더 좋을 것 같아서 제거하였습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server_monitor_list 제거(위 커멘트 참고) 시 다음 명칭이 더 적합해 보일 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정하였습니다.