[CBRD-25436] Creating monitoring thread and data structures for cub_server monitoring #5293

YeunjunLee · 2024-07-02T01:57:21Z

http://jira.cubrid.org/browse/CBRD-25436

cub_server 모니터링을 위한 자료 구조와 server_monitor_thread 생성

server_monitor클래스 생성
- inner class로 server_entry와 server_monitor_list를 가짐
- server_monitor_list 에 server_entny를 삽입/제거/탐색 하기 위한 class 함수 구현
- server_monitor_list 의 특정 server_entry에 대해 server 재구동을 하기 위한 함수 구현 (내부 구현은 CBRD-25438 이슈에서 구현)
server_entry 클래스 생성
- server에서 넘겨 받은 정보를 통해 server_entry를 생성하기 위한 server_entry 생성자 구현
- argv 해제를 위한 소멸자 구현
server_monitor_list 클래스 생성
- 생성자를 통해 revive에 필요한 system parameter들 저장
server_monitor_thread 생성
- server_monitor의 멤버 변수
- server_monitor 소멸자에서 server_monitor_thread 종료 후, server_monitor_list 삭제
master 구동 시 server_monitor 생성
- 현재 issue애서는 server_monitor는 non-HA 환경에서만 생성됨
- sub-task 들에 대한 구현이 끝난 후, HA 환경에서 server_monitor 생성에 대한 구현 진행 예정

src/connection/server_revive.cpp

sa/CMakeLists.txt

src/connection/server_revive.cpp

src/executables/master.c

src/connection/server_revive.hpp

…master_monitor_thread as inner class of it

hornetmj

아직 리뷰 진행 중 입니다. 명일 출근 후 이어서 진행하겠습니다.

hornetmj · 2024-07-04T06:44:23Z

cubrid/CMakeLists.txt

@@ -182,6 +182,7 @@ set(CONNECTION_SOURCES
  ${CONNECTION_DIR}/server_support.c
  ${CONNECTION_DIR}/connection_support.c
  ${CONNECTION_DIR}/host_lookup.c
+  ${CONNECTION_DIR}/server_revive.cpp


cub_master에서만 사용된다면, 포함시킬 필요는 없습니다. cub_master는 cs 라이브러리만 링크합니다.

$ ldd cub_master | grep libcub libcubridcs.so.11.4 => /home/hornetmj/work/cubrid/build_x86_64_debug/_install/CUBRID/lib/libcubridcs.so.11.4 (0x00007fc7c2b58000)

cubrid/CMakeList에서 제거 하였습니다.

src/connection/server_revive.hpp

src/connection/server_revive.cpp

src/executables/master.c

src/connection/server_revive.hpp

src/executables/master.c

src/connection/server_revive.cpp

src/connection/server_revive.hpp

src/connection/server_revive.cpp

hornetmj · 2024-07-05T07:40:10Z

src/connection/server_revive.cpp

+  std::strcpy (this->exec_path, exec_path);
+
+  proc_make_arg (argv, args);
+  this->argv = argv;


지역 변수를 참조하고 있습니다.

수정하였습니다.

hornetmj · 2024-07-05T07:42:40Z

src/connection/server_revive.cpp

+ *   argv(in):
+ */
+void
+proc_make_arg (char **arg, char *args)


master_monitor::server_entry 내부 클래스에서만 사용된다면, private 클래스 함수로 만드는게 어떨까요?

server_monitor::server_entry 내부의 클래스 함수로 변경하였습니다.

src/connection/server_revive.hpp

src/connection/server_revive.cpp

…r variables

joohok

LGTM. 깔끔해졌습니다.
사소한 comment만 남겼습니다. 구조 변경이 없다면, re-request 안해주셔도 됩니다.

joohok · 2024-07-08T07:49:41Z

src/executables/master.c

+    // *INDENT-OFF*
+      server_monitor& server_monitoring_manager = server_monitor::get_instance();
+    // *INDENT-ON*


Suggested change

// *INDENT-OFF*

server_monitor& server_monitoring_manager = server_monitor::get_instance();

// *INDENT-ON*

// *INDENT-OFF*

server_monitor& server_monitoring_manager = server_monitor::get_instance();

// *INDENT-ON*

시작 지점 맞춰주는게 좋을 것 같아요.

src/executables/master_server_monitor.cpp

hornetmj · 2024-07-08T06:58:50Z

src/executables/master.c

+  //        CBRD-24741 is done, this condition will be removed. And server_monitor will be initialized wh
+  //        en first non-HA server is started in HA environment. (server_monitor will be finalized when last
+  //        non-HA server is stopped in HA environment.)
+  if (HA_DISABLED ())


master_util_config_startup()에서 sysprm_load_and_init() 호출을 통해 시스템 파라미터 관련된 모듈이 초기화 되기 때문에 HA_DISABLED() 호출이 정상적으로 동작하지 않을 것 입니다. main 함수 아래 쪽에 HA_DISABLED() 조건 검사 후 hb_master_init()를 호출해 주는 코드부가 있습니다. 해당 위치로 옮겨주시면 좋겠네요.

위치 변경하였습니다.

src/executables/master_server_monitor.hpp

hornetmj · 2024-07-08T07:00:54Z

src/executables/master.c

 #include "config.h"

 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
 #include <time.h>
+#include <memory>


필요 없어 보이네요?

Suggested change

#include <memory>

제거하였습니다.

hornetmj · 2024-07-08T07:03:53Z

src/executables/master_server_monitor.hpp

+#include "connection_defs.h"
+
+#ifdef __cplusplus
+extern "C"


별도의 API 함수를 제공할 계획인가요? 단일 인스턴스를 통해 접근 시 필요해 보이진 않습니다.

이전 코드의 잔재입니다. 제거하였습니다.

hornetmj · 2024-07-08T07:16:06Z

src/executables/master_server_monitor.hpp

+    class server_entry;
+    class server_monitor_list;


class server_entry; 불필요한 전방 선언이며, using server_entry_uptr_t를 server_entry class 정의 뒤 쪽으로 옮기면, class server_monitor_list; 역시 제거 가능

Suggested change

class server_entry;

class server_monitor_list;

class server_entry;

class server_monitor_list;

수정하였습니다.

hornetmj · 2024-07-08T08:19:22Z

src/executables/master_server_monitor.hpp

+	timeval m_last_revive_time;
+	bool m_need_revive;
+
+      private:


NIT:

가독성을 위해 변수를 가장 마지막에 배치하고, 함수는 생성자 등에 이어서 배치하는게 어떨까요?

각 클래스에 대해, 생성자-함수-변수 순으로 정의하도록 전체적으로 배치를 정리하였습니다.

hornetmj · 2024-07-08T08:22:15Z

src/executables/master_server_monitor.hpp

+	void proc_make_arg (char *args);
+    };
+
+    class server_monitor_list


server_monitor_list를 별도의 내부 클래스로 유지하는 목적이 있을까요? server_entry를 위한 std::verctor<>는 server_monitor에 위치시켜도 충분할 것 같습니다. using을 이용해서 적당한 타입을 선언해 주면 될 것 같구요.

추후에 server_monitor_list 에 대한 탐색/삽입/제거/서버 revive 관련 함수들을 배치할 때, server_monitor class에 배치시키기 보다는 list에 대한 함수로서 배치하는 것이 더 보기 좋아보인다고 생각해서 별도의 클래스로 설계하였습니다.

std::vector 자체가 list의 의미를 담고 있습니다. 다음 경우가 아니라면 크게 의미는 없지 않을까요?

server_monitor가 여러개의 server_monitor_list를 만들어서 사용해야 하는 경우

server_monitor_list 내부의 구조를 숨기고 싶은 경우

ex) server_monitor_list는 add_server_entry(), remove_server_entry() 인터페이스를 제공하며, server_monitor_list 내부에서 std::vector가 사용됨을 숨긴다. 이후 필요에 따라 std::vector는 다른 컨테이너나 사용자 정의 자료구조로 대체 가능하다.

그렇지 않다면, server_monitor는 std::vector에 접근하기 위해 server_monitor_list에 한번 더 접근해야 하기에 코드 복잡도가 증가합니다. 또한, class 타입 증가, 부모 class 라인수 증가 등의 이슈도 있습니다.

이후 진행될 sub-task에서 server_monitor_list 클래스 사용의 이점이 설명될 수 있다면, 이 리뷰는 해당 이슈에서 추가 논의해도 좋습니다.

server_monitor가 현재 위 두 경우에 해당하는 것 같지는 않습니다. 따라서 말씀 주신 대로, server_monior_list를 별개의 class로 설정하는 것 보다 server_monitor의 멤버로 직접 두는 것이 훨씬 간편할 것 같습니다.

해당 내용 반영하였고, 추가로 server_monitor_list 내부에 저장하고 있던 system parameter들도 사용 시 직접 가져와 사용하는 것이 클래스 구조를 간소화 하는데 더 좋을 것 같아서 제거하였습니다.

hornetmj · 2024-07-08T08:27:04Z

src/executables/master_server_monitor.hpp

+    ~server_monitor ();
+
+    std::unique_ptr<std::thread> m_monitoring_thread;
+    std::unique_ptr<server_monitor_list> m_monitor_list;


server_monitor_list 제거(위 커멘트 참고) 시 다음 명칭이 더 적합해 보일 것 같습니다.

Suggested change

std::unique_ptr<server_monitor_list> m_monitor_list;

std::unique_ptr<server_monitor_list> m_server_entry_list;

수정하였습니다.

hornetmj · 2024-07-08T08:33:02Z

src/executables/master_server_monitor.cpp

+  , m_last_revive_time {0, 0}
+  , m_need_revive {false}
+{
+  proc_make_arg (args);


현재는 null일 것 같은데, null이 아닌 경우에만 호출하는게 어떨까요?

nullptr 조건문을 추가하였습니다.

hornetmj · 2024-07-08T08:35:25Z

src/executables/master_server_monitor.hpp

+	const int m_pid;
+	const std::string m_server_name;
+	const std::string m_exec_path;
+	std::vector<std::string> m_argv;
+	CSS_CONN_ENTRY *m_conn;
+	timeval m_last_revive_time;
+	bool m_need_revive;


각 변수에 대한 설명을 주석으로 남겨주세요. class/struct의 멤버 변수에 대해서는 기본적으로 설명 주석을 남겨주시는게 좋습니다. 너무 명료한 경우엔 남기지 않아도 되겠지만요.

추가하였습니다.

src/executables/master_server_monitor.hpp

src/executables/master.c

hornetmj

master_server_monitor.cpp 파일은 아직 리뷰하지 않았습니다. 남긴 리뷰 내용 결정된 이후에 리뷰 진행하도록 하겠습니다. 답변해 주시면 되겠습니다.

src/executables/master.c

src/executables/master_server_monitor.hpp

hornetmj · 2024-07-10T07:56:28Z

src/executables/master_server_monitor.hpp

+	void proc_make_arg (char *args);
+    };
+
+    class server_monitor_list


std::vector 자체가 list의 의미를 담고 있습니다. 다음 경우가 아니라면 크게 의미는 없지 않을까요?

server_monitor가 여러개의 server_monitor_list를 만들어서 사용해야 하는 경우

server_monitor_list 내부의 구조를 숨기고 싶은 경우

ex) server_monitor_list는 add_server_entry(), remove_server_entry() 인터페이스를 제공하며, server_monitor_list 내부에서 std::vector가 사용됨을 숨긴다. 이후 필요에 따라 std::vector는 다른 컨테이너나 사용자 정의 자료구조로 대체 가능하다.

그렇지 않다면, server_monitor는 std::vector에 접근하기 위해 server_monitor_list에 한번 더 접근해야 하기에 코드 복잡도가 증가합니다. 또한, class 타입 증가, 부모 class 라인수 증가 등의 이슈도 있습니다.

이후 진행될 sub-task에서 server_monitor_list 클래스 사용의 이점이 설명될 수 있다면, 이 리뷰는 해당 이슈에서 추가 논의해도 좋습니다.

src/executables/master_server_monitor.hpp

src/executables/master.c

joohok · 2024-07-10T09:09:01Z

src/executables/master.c

+  if (HA_DISABLED ())
+    {
+      // *INDENT-OFF*
+      master_server_monitor.reset(new server_monitor());


Suggested change

master_server_monitor.reset(new server_monitor());

master_server_monitor.reset (new server_monitor ());

수정하였습니다.

src/executables/master_server_monitor.hpp

joohok · 2024-07-10T15:38:06Z

src/executables/master_server_monitor.cpp

+  fflush (stdout);
+}
+
+// In server_monitor destructor, it should guerentee that


Suggested change

// In server_monitor destructor, it should guerentee that

// In server_monitor destructor, it should guarantee that

수정하였습니다.

src/executables/master_server_monitor.cpp

src/executables/master_server_monitor.hpp

src/executables/master_server_monitor.cpp

hornetmj · 2024-07-11T04:03:05Z

src/executables/master_server_monitor.cpp

+  m_thread_shutdown = false;
+
+  m_server_entry_list = std::make_unique<std::vector<server_entry>> ();
+  fprintf (stdout, "server_monitor_list is created. \n");


Suggested change

m_thread_shutdown = false;

m_server_entry_list = std::make_unique<std::vector<server_entry>> ();

fprintf (stdout, "server_monitor_list is created. \n");

m_server_entry_list = std::make_unique<std::vector<server_entry>> ();

fprintf (stdout, "server_entry_list is created. \n");

m_thread_shutdown = false;

수정하였습니다.

메세지 출력 시 server_monitor_list 아닌 server_entry_list로 제안드렸습니다. 제안된 내용을 그대로 반영하시려면, Commit suggestion 버튼을 이용하시면 됩니다.

server_entry_list 생성 / 제거 디버깅 메시지 둘 다 수정하였습니다.

src/executables/master_server_monitor.cpp

hornetmj · 2024-07-11T04:22:36Z

src/executables/master_server_monitor.cpp

+server_monitor::~server_monitor ()
+{
+  m_thread_shutdown = true;
+  if (m_monitoring_thread->joinable())
+    {
+      m_monitoring_thread->join();
+      fprintf (stdout, "server_monitor_thread is terminated. \n");
+    }
+  fprintf (stdout, "server_monitor_list is deleted. \n");
+  fflush (stdout);
+}


Suggested change

server_monitor::~server_monitor ()

{

m_thread_shutdown = true;

if (m_monitoring_thread->joinable())

{

m_monitoring_thread->join();

fprintf (stdout, "server_monitor_thread is terminated. \n");

}

fprintf (stdout, "server_monitor_list is deleted. \n");

fflush (stdout);

}

server_monitor::~server_monitor ()

{

m_thread_shutdown = true;

if (m_monitoring_thread->joinable())

{

m_monitoring_thread->join();

fprintf (stdout, "server_monitor_thread is terminated. \n");

}

assert (m_server_entry_list->size () == 0);

fprintf (stdout, "server_entry_list is deleted. \n");

fflush (stdout);

}

수정하였습니다.

메세지 출력 시 server_monitor_list 아닌 server_entry_list로 제안드렸습니다. 제안된 내용을 그대로 반영하시려면, Commit suggestion 버튼을 이용하시면 됩니다.

server_entry_list 생성 / 제거 디버깅 메시지 둘 다 수정하였습니다.

src/executables/master_server_monitor.cpp

hornetmj

출력 메시지 관련해서 커멘트 남겼고, 제안(suggestion) 받은 코드를 PR 상에서 바로 반영하는 방법에 대해 커멘트 남겼습니다.

YeunjunLee added 3 commits July 1, 2024 18:29

add data structures and worker thread

9216977

remove debugging logs

5fa32d1

indent

53d83fd

YeunjunLee requested review from hornetmj, H2SU and joohok July 2, 2024 01:57

indent cpp

1214fb9

YeunjunLee marked this pull request as ready for review July 2, 2024 02:09

YeunjunLee requested a review from beyondykk9 as a code owner July 2, 2024 02:09

YeunjunLee changed the base branch from develop to feature/server_revive_2 July 2, 2024 02:22

YeunjunLee added 2 commits July 2, 2024 11:25

remove debugging log in master_monitoring_list

e35c2a8

remove whitespace

27144c4

joohok reviewed Jul 2, 2024

View reviewed changes

YeunjunLee marked this pull request as draft July 2, 2024 08:38

YeunjunLee added 2 commits July 3, 2024 18:17

make master_monitor class and put server_entry, master_monitor_list, …

29a41b4

…master_monitor_thread as inner class of it

modify licence

dc02a08

YeunjunLee marked this pull request as ready for review July 3, 2024 09:37

indent

6c04ca7

YeunjunLee changed the title ~~[CBRD-25436] cub_server 모니터링을 위한 자료 구조와 monitoring thread 생성~~ [CBRD-25436] Creating monitoring thread and data structures for cub_server monitoring Jul 4, 2024

hornetmj reviewed Jul 4, 2024

View reviewed changes

joohok reviewed Jul 5, 2024

View reviewed changes

src/connection/server_revive.cpp Outdated Show resolved Hide resolved

src/executables/master.c Outdated Show resolved Hide resolved

src/executables/master.c Outdated Show resolved Hide resolved

src/connection/server_revive.hpp Outdated Show resolved Hide resolved

hornetmj reviewed Jul 5, 2024

View reviewed changes

joohok reviewed Jul 8, 2024

View reviewed changes

src/connection/server_revive.hpp Outdated Show resolved Hide resolved

src/connection/server_revive.hpp Outdated Show resolved Hide resolved

src/connection/server_revive.hpp Outdated Show resolved Hide resolved

src/connection/server_revive.cpp Outdated Show resolved Hide resolved

YeunjunLee added 6 commits July 8, 2024 13:42

Improve class structure, Utilizaing unique ptr, Change names of membe…

93476d6

…r variables

remove undeleted code

bd0e96b

remove undeleted code

6973275

remove undeleted code

419557b

remove undeleted code

c507718

fix while loop

5d7dc21

YeunjunLee requested a review from hornetmj July 8, 2024 05:09

Remove whitespace

503ede2

joohok approved these changes Jul 8, 2024

View reviewed changes

Modify some comments

72c60eb

hornetmj reviewed Jul 8, 2024

View reviewed changes

YeunjunLee added 7 commits July 8, 2024 18:19

Add comments for member variables

96b0ebd

Add delete_instance, Change m_thread_shutdown to volatile

a0b20c4

Align comments

d3c8708

Make constructor of server_monitor as private

d842e7c

Remove m_mutex in server_monitor

e838efa

Change singleton pattern from unique_ptr to static

7e3f2df

Change server_monitor from singleton pattern to global variable

4d0a6ca

YeunjunLee requested review from hornetmj and joohok July 10, 2024 06:38

joohok reviewed Jul 10, 2024

View reviewed changes

src/executables/master_server_monitor.hpp Outdated Show resolved Hide resolved

joohok reviewed Jul 10, 2024

View reviewed changes

src/executables/master.c Outdated Show resolved Hide resolved

Disable formatting

9e47e76

hornetmj reviewed Jul 10, 2024

View reviewed changes

joohok approved these changes Jul 10, 2024

View reviewed changes

YeunjunLee added 3 commits July 11, 2024 10:43

Remove master_monitor_list inner class

f46fe82

Change position of global variable

a7be8ff

Fix typo

daee3e1

YeunjunLee requested a review from hornetmj July 11, 2024 02:01

hornetmj reviewed Jul 11, 2024

View reviewed changes

joohok reviewed Jul 11, 2024

View reviewed changes

src/executables/master_server_monitor.cpp Outdated Show resolved Hide resolved

YeunjunLee added 2 commits July 11, 2024 16:40

Add assertion in destructor of server_monitor

9cdbea3

Change debugging message

5b9f8cd

hornetmj approved these changes Jul 11, 2024

View reviewed changes

YeunjunLee removed the request for review from beyondykk9 July 11, 2024 08:48

hornetmj merged commit bcb80f6 into CUBRID:feature/server_revive_2 Jul 12, 2024
7 of 9 checks passed

	std::unique_ptr<server_monitor_list> m_monitor_list;
	std::unique_ptr<server_monitor_list> m_server_entry_list;

	master_server_monitor.reset(new server_monitor());
	master_server_monitor.reset (new server_monitor ());

	// In server_monitor destructor, it should guerentee that
	// In server_monitor destructor, it should guarantee that

[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

Conversation

YeunjunLee commented Jul 2, 2024 • edited Loading

hornetmj left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joohok left a 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hornetmj left a 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

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

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

hornetmj left a comment

Choose a reason for hiding this comment

YeunjunLee commented Jul 2, 2024 •

edited

Loading