Refactoring ProxySQL Monitor Module #4063

rahim-kanji · 2022-12-21T12:54:03Z

Refactoring ProxySQL Monitor Module

Asynchronous handling of Ping, Group replication, Replication Lag, Read Only and Galera if MySQL connection is available, else tasks will be delegated to Consumer Thread.

Added separate init code for async task handling. Code cleanup

* Initialized few uninitialized variable.

JavierJF

This is an initial review with feedback to open discussion, the are still parts that I would like to look into more detail for these changes, so I wouldn't call it a final review. But I think there is enough feedback to start a conversation about it.

lib/MySQL_Monitor.cpp

JavierJF · 2022-12-23T15:24:22Z

lib/MySQL_Monitor.cpp

+	}
+
+
+	bool event_loop(int poll_timeout) {


See previous comment on Monitor_Poll class definition.

lib/MySQL_Monitor.cpp

include/MySQL_Monitor.hpp

JavierJF · 2022-12-25T18:45:56Z

lib/MySQL_Monitor.cpp

-					assert(my!=my1);
-					assert(my->net.fd!=my1->net.fd);
-				}
+				if (my) {


Could you elaborate on why this change is now required? I faced this during testing, but haven't had time for reasoning fully about it. A nice comment pointing to why this check is now required when checking for the MYSQL connections objects would be great!

If my is NULL in debug, it will crash here
assert(my->net.fd!=my1->net.fd);

Yes! I experienced the same, that is what I meant by facing it during testing. But was asking if you knew the reason for that to happen in this rework but not in the previous impl, I haven't reasoned a lot about it yet.

Previously, some work was done in get_connection to close connection if not used for long time. It can now return null if there are no open connections left in container.

Thanks for the clarification, there is still missing a comment in this part of the code for it, I know this change was already merged, but the comment is missing, if you could please add it that would be great.

Comment added

lib/MySQL_Monitor.cpp

include/MySQL_Monitor.hpp

rahim-kanji · 2022-12-29T06:58:54Z

retest this please

lib/MySQL_Monitor.cpp

JavierJF · 2022-12-29T11:48:46Z

lib/MySQL_Monitor.cpp

-					assert(my!=my1);
-					assert(my->net.fd!=my1->net.fd);
-				}
+				if (my) {


Yes! I experienced the same, that is what I meant by facing it during testing. But was asking if you knew the reason for that to happen in this rework but not in the previous impl, I haven't reasoned a lot about it yet.

lib/MySQL_Monitor.cpp

* Break loop if shutdown flag is true. * Removed duplicate success flag and exit label. * Few typo fixed.

mirostauder · 2023-01-05T15:03:01Z

retest this please

mirostauder · 2023-01-05T20:23:20Z

retest this please

mirostauder · 2023-01-06T08:32:11Z

retest this please

mirostauder · 2023-01-21T16:16:19Z

retest this please

JavierJF

Looking great. I think after a couple of points mentioned in this last comments, this is ready for merge!

JavierJF · 2023-01-23T12:20:04Z

include/MySQL_Monitor.hpp

+		TASK_RESULT_SUCCESS = Task executed successfully.
+		TASK_RESULT_PENDING = Task is in pending state.
+	*/
+	MySQL_Monitor_State_Data_Task_Result task_handler(short event_, short& wait_event);


Please try to follow the Doxygen convention used in the rest of the project for comments, it's note required when adding notes, but for definitions or declarations would really help in the later run if everything is consistent.

include/MySQL_Monitor.hpp

JavierJF · 2023-01-23T12:21:58Z

include/MySQL_Monitor.hpp

+	// ** TASK_RESULT_TIMEOUT = mysql connection will be closed and error log will be generated.
+	// ** TASK_RESULT_FAILED =  mysql connection will be closed and error log will be generated.
+	// Note: calling init_async is mandatory before executing tasks asynchronously.
+	void monitor_ping_async(SQLite3_result* resultset);


Same comment as for task_handler regarding documentation style. Obviously you don't need to repeat the same info in every handler function, but you can just point to your detailed comment in the others.

JavierJF · 2023-01-23T12:23:06Z

lib/MySQL_Monitor.cpp

-					assert(my!=my1);
-					assert(my->net.fd!=my1->net.fd);
-				}
+				if (my) {


Thanks for the clarification, there is still missing a comment in this part of the code for it, I know this change was already merged, but the comment is missing, if you could please add it that would be great.

lib/MySQL_Monitor.cpp

JavierJF · 2023-01-23T14:04:28Z

lib/MySQL_Monitor.cpp

+}
+
+void MySQL_Monitor_State_Data::mark_task_as_timeout(unsigned long long time) {
+


There are several places that hold this extra empty lines after declarations or if statements, but only happens sometimes, I don't know if there is a reason for them. Just mentioning it in case it's an editor thing, because they look "random".

It's done intentionally to avoid code clutter.

JavierJF · 2023-01-23T14:05:29Z

lib/MySQL_Monitor.cpp

+#define NEXT_IMMEDIATE(new_st) do { async_state_machine_=new_st; goto __again; } while (0)
+
+short MySQL_Monitor_State_Data::next_event(MDB_ASYNC_ST new_st, int status) {
+


Same kind of empty line as mentioned in other comments.

It's done intentionally to avoid code clutter.

JavierJF · 2023-01-23T14:05:54Z

lib/MySQL_Monitor.cpp

+		ready_tasks.reserve(tasks_to_process_count);
+
+		while (len_) {
+


Empty line as mentioned in other comments.

It's done intentionally to avoid code clutter.

JavierJF · 2023-01-23T14:32:23Z

lib/MySQL_Monitor.cpp

+
+	std::vector<std::unique_ptr<MySQL_Monitor_State_Data>> mmsds;
+	mmsds.reserve(resultset->rows_count);
+	Monitor_Poll monitor_poll(resultset->rows_count);


I have a suggestion for improving the the usage pattern of Monitor_Poll class, which I think it's a little odd right now regarding resource handling, as it could take much better advantage of the already used std::unique_ptr.

As I see, the usage pattern right now in the PR for Monitor_Poll is delegating the resource destruction of the created MySQL_Monitor_State_Data over a std::vector<std::unique_ptr<MySQL_Monitor_State_Data>>. This vector looks unused just for performing this resource handling, this creates a window for potential missuses of the class, since the manipulation of the data stored in the std::unique_ptr<> doesn't happen in the same scope in which the std::unique_ptr<> lives, instead, get() is used, invalidating the protection offered by the container. You know, I think this is normally fine when passing the pointer to a legacy function (system API, etc...) that is going to return in the same scope, but in this case, we are creating another container with the results of this get() which can have a completely different life-spam to the original std::unique_ptr<>.

What I suggest is to make this already constructed vector an argument for Monitor_Poll constructor, using move semantics for the initialization of an internal member. The size() member of the vector can be used for the same purpose as it's now used in the current constructor, initializing the real internal structure used for polling, and iterating it as it's done right now for calling: monitor_poll.add((POLLIN|POLLOUT|POLLPRI), mmsd.get()). There is no reason for even changing the internal logic for add, since its purpose will be the same, but since it won't have any external usage, we should probably declare it as private. With this change Monitor_Poll will be well-formed resource wise since its construction, without worrying about scopes for the MySQL_Monitor_State_Data.

Monitor Poll has aggregation type association with mmsd. Putting vector inside monitor poll class will change this association. Regarding mmsd life-span, this will never outlive vector, at-least in this current design.

# Conflicts: # lib/MySQL_Monitor.cpp

rahim-kanji added 4 commits December 21, 2022 17:52

Refactoring ProxySQL Monitor Module

1a47d98

Asynchronous handling of Ping, Group replication, Replication Lag, Read Only and Galera if MySQL connection is available, else tasks will be delegated to Consumer Thread.

Refactoring ProxySQL Monitor Module

147d23a

Added separate init code for async task handling. Code cleanup

Avoid null pointer mysql connection access in Debug

7fa44ce

* Added total poll timeout in case poll is waiting on stale connection.

ebc7ebf

* Initialized few uninitialized variable.

JavierJF reviewed Dec 25, 2022

View reviewed changes

Few optimizations and fixes

6da9271

JavierJF reviewed Dec 29, 2022

View reviewed changes

JavierJF reviewed Dec 30, 2022

View reviewed changes

lib/MySQL_Monitor.cpp Show resolved Hide resolved

lib/MySQL_Monitor.cpp Show resolved Hide resolved

rahim-kanji added 2 commits December 30, 2022 18:13

* Moved task_handler code from .hpp to .cpp

821a6f4

* Break loop if shutdown flag is true. * Removed duplicate success flag and exit label. * Few typo fixed.

* Bulk processing of ready tasks based on provided percentage

11be1f1

Added proxysqltest: monitor task forced timeout

340a4f3

JavierJF reviewed Jan 23, 2023

View reviewed changes

rahim-kanji added 2 commits January 25, 2023 20:05

* Added few comments and fixed comment style

3bb4bcf

Resolve conflicts

ae40be4

# Conflicts: # lib/MySQL_Monitor.cpp

rahim-kanji marked this pull request as ready for review January 25, 2023 15:11

renecannao merged commit cedbad5 into v2.x Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring ProxySQL Monitor Module #4063

Refactoring ProxySQL Monitor Module #4063

rahim-kanji commented Dec 21, 2022 •

edited

Loading

JavierJF left a comment

JavierJF Dec 23, 2022

JavierJF Dec 25, 2022

rahim-kanji Dec 27, 2022

JavierJF Dec 29, 2022

rahim-kanji Dec 30, 2022

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

rahim-kanji commented Dec 29, 2022

JavierJF Dec 29, 2022

mirostauder commented Jan 5, 2023

mirostauder commented Jan 5, 2023

mirostauder commented Jan 6, 2023

mirostauder commented Jan 21, 2023

JavierJF left a comment

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

JavierJF Jan 23, 2023

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

JavierJF Jan 23, 2023

rahim-kanji Jan 25, 2023

		}

		void MySQL_Monitor_State_Data::mark_task_as_timeout(unsigned long long time) {

		#define NEXT_IMMEDIATE(new_st) do { async_state_machine_=new_st; goto __again; } while (0)

		short MySQL_Monitor_State_Data::next_event(MDB_ASYNC_ST new_st, int status) {

Refactoring ProxySQL Monitor Module #4063

Refactoring ProxySQL Monitor Module #4063

Conversation

rahim-kanji commented Dec 21, 2022 • edited Loading

JavierJF 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

rahim-kanji commented Dec 29, 2022

Choose a reason for hiding this comment

mirostauder commented Jan 5, 2023

mirostauder commented Jan 5, 2023

mirostauder commented Jan 6, 2023

mirostauder commented Jan 21, 2023

JavierJF 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

rahim-kanji commented Dec 21, 2022 •

edited

Loading