feat(server): support for monitor command. This include some code #412

boazsade · 2022-10-20T07:08:41Z

refactor for the request message to dispatch async. (#344)

Signed-off-by: Boaz Sade [email protected]

Supporting monitor command
This is still WIP.
Need to add support for ADMIN level commands
Need to correct the formatting of the output.

Monitor command is handled.
Refactor the the async message dispatch to support different types of messages.

romange

I liked how you structured code but I think it currently has memory leaks.

also, please remove "This include some code" from the commit title :)

romange · 2022-10-20T08:49:16Z

src/facade/conn_context.h

+  bool replica_conn : 1;
+  bool authenticated : 1;
+  bool force_dispatch : 1;   // whether we should route all requests to the dispatch fiber.
+  bool monitor : 1 = false;  // when the connection is monitor do not support most commands other


i feel uncomfortable to initialize bits here, also it's not consistent with the rest of the fields.

romange · 2022-10-20T08:54:48Z

src/server/conn_context.cc

+  owner()->SendMonitorMsg(msg, borrows);
+}
+
+void ConnectionContext::MonitorCmd() {


I think the right place to implement MonitorCmd is inside the main service.

src/server/conn_context.cc

src/facade/dragonfly_connection.cc

romange · 2022-10-20T09:38:00Z

src/facade/dragonfly_connection.cc

+// Used as custom deleter for Request object
+struct Connection::RequestDeleter {
+  void operator()(Request* req) const {
+    mi_free(req);


i like the idea of custom delete but i am pretty sure you have a memory leak here.

I checked for memory leaks. I didn't find any. any pointer that was allocated, was released as well (or at least the call to released it was called, if mi_free don't really free it, then we would have a memory leak here), but this is just the same as the old code. There is a single allocation of memory to the req itself. I can add an explicit call to dtor, but its trivial.

it's not trivial at all. it has non-pod members that by themselves take memory. for example storage member is leaked for requests that take more than 120 bytes. If I am correct, it could be easily proven by sending a pipelined traffic to the server.

This is why I added the explicit call to the dtor in the code. So that if we have some object with none trivial dtor it would be called. Right now the code does the same as it did before, only we don't need to call it explicitly since it is done in the deleter of the unique ptr

src/facade/dragonfly_connection.cc

romange · 2022-10-20T09:44:23Z

src/facade/dragonfly_connection.h

@@ -116,11 +125,16 @@ class Connection : public util::Connection {
  std::unique_ptr<ConnectionContext> cc_;

  struct Request;
+  struct DispatchOperations;


Unless I missed anything, DispatchOperations and DispatchCleanup could reside in cc file and do not have to be part of the connection class.

I need them there because they require access to the private part of the enclosing class. I tried otherwise and failed to compile it. They are not really part of the interface since you cannot use them from the outside and they are in the private part of the class.

ok, i missed that

romange · 2022-10-20T15:37:32Z

src/facade/dragonfly_connection.h

@@ -19,6 +19,10 @@
 typedef struct ssl_ctx_st SSL_CTX;
 typedef struct mi_heap_s mi_heap_t;

+namespace dfly {
+class ConnectionState;


this creates a circular linking dependency.
The way the code is designed, "server" is dependent on "core" and "facade". "facade" can not be dependent on the server code. I bet the "ok_backend" target does not build anymore.

So I need to make sure that I do not use server from inside facade, I will make the required changes for that

romange · 2022-10-21T06:26:19Z

src/facade/dragonfly_connection.cc

@@ -263,21 +391,30 @@ void Connection::RegisterOnBreak(BreakerCb breaker_cb) {
  breaker_cb_ = breaker_cb;
 }

-void Connection::SendMsgVecAsync(const PubMessage& pub_msg, fibers_ext::BlockingCounter bc) {
+void Connection::SendMonitorMsg(std::string_view monitor_msg,
+                                util::fibers_ext::BlockingCounter bc) {


why do you pass "bc" all the way from MonitorCmd to here?

This is the same way we are doing it in the pub/sub support. This sync all the used connection to the one location where it is used. Maybe we don't need this in this case? let talk about this directly?

…now support monitor type as well. (dragonflydb#344) feat(server): monitor command, add command filters feat(server): support for monitor command. spport for auth not showing sensitive data (dragonflydb#344) feat(server): monitor command output format support none pritable char as well feat(server): monitor command - move monitor handling after verify and run other command, add cleanup when connection closed (dragonflydb#344) feat(server): monitor command - disable by default (dragonflydb#344) Signed-off-by: Boaz Sade <[email protected]>

boazsade · 2022-10-23T08:42:07Z

I would create two PRs for this. One about the refactor to the connection in regard to the message dispatching. The second about the monitor command.

romange reviewed Oct 20, 2022

View reviewed changes

boazsade force-pushed the support-monitor-command branch from 6b75e5d to b15b564 Compare October 20, 2022 14:34

romange reviewed Oct 20, 2022

View reviewed changes

boazsade force-pushed the support-monitor-command branch from b15b564 to 96f90ec Compare October 21, 2022 05:39

boazsade marked this pull request as ready for review October 21, 2022 05:40

romange reviewed Oct 21, 2022

View reviewed changes

boazsade force-pushed the support-monitor-command branch from 96f90ec to 0e7e837 Compare October 21, 2022 09:30

boazsade closed this Oct 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): support for monitor command. This include some code #412

feat(server): support for monitor command. This include some code #412

boazsade commented Oct 20, 2022

romange left a comment

romange Oct 20, 2022

boazsade Oct 20, 2022

romange Oct 20, 2022

boazsade Oct 20, 2022

romange Oct 20, 2022

boazsade Oct 20, 2022

romange Oct 20, 2022 •

edited

Loading

boazsade Oct 23, 2022

romange Oct 20, 2022

boazsade Oct 20, 2022

romange Oct 20, 2022

romange Oct 20, 2022

boazsade Oct 23, 2022

romange Oct 21, 2022

boazsade Oct 23, 2022

boazsade commented Oct 23, 2022

feat(server): support for monitor command. This include some code #412

feat(server): support for monitor command. This include some code #412

Conversation

boazsade commented Oct 20, 2022

romange 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

romange Oct 20, 2022 • edited Loading

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

boazsade commented Oct 23, 2022

romange Oct 20, 2022 •

edited

Loading