Logger: Add reporting capabilities to the WBM #556

Yuval-Ariel · 2023-06-13T09:39:46Z

This commit adds reporting capabilities to the WBM. Specifically, for when it sets or resets a delay request to the WriteController. This is done by adding the Logger to the WBM during DB Open, in the Ctor of ColumnFamilySet together with the WC.

Yuval-Ariel · 2023-06-13T09:41:25Z

@udi-speedb , can we review the initial code which handles just the WBM and then i'll add the WC part for a second review?

udi-speedb · 2023-06-13T09:43:52Z

@udi-speedb , can we review the initial code which handles just the WBM and then i'll add the WC part for a second review?

Sure, but it will have to wait until I am done with the log parser's documentation update

mrambacher

Without a design doc, this is very hard to review but it does not seem to fit into the current design of the WBM. Why not use something like the StallInterface to accomplish what you want rather than add an entire new relationship? The WBM already has a relationship to the StallInterface for each DB.

I am not in favor of making more classes (WriteController) public, as I think this just adds to the confusion of managing options.

mrambacher · 2023-06-19T12:50:30Z

include/rocksdb/write_buffer_manager.h

+                           std::string db_session_id);
+  void DeregisterWCAndLogger(std::shared_ptr<WriteController> wc,
+                             std::shared_ptr<Logger> logger,
+                             std::string db_session_id);


Rather than change the public interface, why not add logger and db_session_id to the WriteController?

for reporting WC related things (in WBMSetupDelay and ResetDelay) that would indeed work but i'd like to add the loggers to the WBM independently of the WCs so that it could also be used for other purposes.

mrambacher · 2023-06-19T12:50:53Z

include/rocksdb/write_buffer_manager.h

+  void RegisterWCAndLogger(std::shared_ptr<WriteController> wc,
+                           std::shared_ptr<Logger> logger,
+                           std::string db_session_id);


The standard used by RocksDB would be to pass all of these as const &

the overhead of creating another copy (which would probably be elided) once every db open is too insignificant and passing it by value indicates that we share ownership (even if the function itself doesnt need to be a part of the lifecycle). this also breaks the guarantee of const as later on the WC wont be used as const.

This does not break any const rules, and is in fact standard practice. The const applies to the reference, not to what it is applied to. If you have a const& shared_ptr foo, you can call all methods off of T whether they are const or not. What you cannot do is change foo to point to some other object.

If we take the case of "std::string session_id" -- which has the same rules and semantics, this means you can do any method off of session_id -- even if it is a "const std::string &". The one thing you cannot do is set session_id to something else. In this case (for both logger and session_id) setting the values to something else makes no sense (I do not plan to return the new value, else I would have used a non-const reference) -- which means good code hygiene and semantics say these should be const references.

true but - http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r36-take-a-const-shared_ptrwidget-parameter-to-express-that-it-might-retain-a-reference-count-to-the-object-
clearly doesnt fit with our use case of passing the WC and logger to share ownership.

true but - http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r36-take-a-const-shared_ptrwidget-parameter-to-express-that-it-might-retain-a-reference-count-to-the-object- clearly doesnt fit with our use case of passing the WC and logger to share ownership.

The example in those guidelines is not the same. Those guidelines pass the object within the shared ptr to a function, not the shared pointer.

The Google Coding Guide states that parameters that are not modified (which this is an example of). should be declared as const. If you search the public API for RocksDB, almost every example uses const shared_ptr&. Even look at the use of Logger where data is clearly written to the logger -- the input is a const shared_ptr&, because the shared pointer is not modified.

The point is that i want to show in the code that my intent in passing the shared_ptr here by value is that it shares ownership with the callee. But i think we've discussed this enough, it doesnt really matter that much to me

Yuval-Ariel · 2023-06-20T13:36:19Z

Without a design doc, this is very hard to review but it does not seem to fit into the current design of the WBM. Why not use something like the StallInterface to accomplish what you want rather than add an entire new relationship? The WBM already has a relationship to the StallInterface for each DB.

I believe you are referring to the changes done under #392 , so im not sure this PR is the place to rethink what was done there but in any case, the StallInterface is not suitable since it only allows completely stopping the writes and also it would clash with the WriteContoller delay settings.

I am not in favor of making more classes (WriteController) public, as I think this just adds to the confusion of managing options.

i agree this is an issue but i dont see a better option to replace what #392 offers.

mrambacher · 2023-06-21T15:12:32Z

There is already a concept of a StallInterface that is internal to the WBM. You could extend this interface and add more different implementations.

If you write up what the problem is you are trying to solve somewhere and how you plan to solve it, others can weigh in on the approach and give ideas.

Yuval-Ariel · 2023-06-22T06:16:54Z

There is already a concept of a StallInterface that is internal to the WBM. You could extend this interface and add more different implementations.

your comments are a bit late since they are intended for #346 .. this PR deals with enabling the WBM and WC to report on internal operations which are relevant for the dbs that are using them. If the issues you have with #346 prevent you from further reviewing this PR then i suggest you open a new issue that deals with that. thanks

If you write up what the problem is you are trying to solve somewhere and how you plan to solve it, others can weigh in on the approach and give ideas.

I assume you havent read the issue description but it clearly states that the problem is that the WBM and WC have no access to the logger.

mrambacher · 2023-06-23T13:13:31Z

There is already a concept of a StallInterface that is internal to the WBM. You could extend this interface and add more different implementations.

your comments are a bit late since they are intended for #346 .. this PR deals with enabling the WBM and WC to report on internal operations which are relevant for the dbs that are using them. If the issues you have with #346 prevent you from further reviewing this PR then i suggest you open a new issue that deals with that. thanks

If you write up what the problem is you are trying to solve somewhere and how you plan to solve it, others can weigh in on the approach and give ideas.

I assume you havent read the issue description but it clearly states that the problem is that the WBM and WC have no access to the logger.

Would it not be simpler and cleaner instead to pass a DB to a Register function? A DB* has all of the properties you require (and more for the next iteration) already grouped into one object. There does not have to be any fancy data structures or multiple maps to handle this, and the registration is already done in a DB class.

udi-speedb · 2023-06-23T13:47:32Z

There is already a concept of a StallInterface that is internal to the WBM. You could extend this interface and add more different implementations.

your comments are a bit late since they are intended for #346 .. this PR deals with enabling the WBM and WC to report on internal operations which are relevant for the dbs that are using them. If the issues you have with #346 prevent you from further reviewing this PR then i suggest you open a new issue that deals with that. thanks

If you write up what the problem is you are trying to solve somewhere and how you plan to solve it, others can weigh in on the approach and give ideas.

I assume you havent read the issue description but it clearly states that the problem is that the WBM and WC have no access to the logger.

Would it not be simpler and cleaner instead to pass a DB to a Register function? A DB* has all of the properties you require (and more for the next iteration) already grouped into one object. There does not have to be any fancy data structures or multiple maps to handle this, and the registration is already done in a DB class.

I am receiving the emails with the comments. I am not a reviewer but I have read your suggestion and would like to shortly say that:
The WBM should NOT be familiar (code-wise) with its users, and shouldn't include db.h in its implementation. It's a lower level infrastructure entity and as such, should be independent of its clients. It has been so far that way and should remain so.

mrambacher · 2023-06-23T14:11:19Z

There is already a concept of a StallInterface that is internal to the WBM. You could extend this interface and add more different implementations.

your comments are a bit late since they are intended for #346 .. this PR deals with enabling the WBM and WC to report on internal operations which are relevant for the dbs that are using them. If the issues you have with #346 prevent you from further reviewing this PR then i suggest you open a new issue that deals with that. thanks

If you write up what the problem is you are trying to solve somewhere and how you plan to solve it, others can weigh in on the approach and give ideas.

I assume you havent read the issue description but it clearly states that the problem is that the WBM and WC have no access to the logger.

Would it not be simpler and cleaner instead to pass a DB to a Register function? A DB* has all of the properties you require (and more for the next iteration) already grouped into one object. There does not have to be any fancy data structures or multiple maps to handle this, and the registration is already done in a DB class.

I am receiving the emails with the comments. I am not a reviewer but I have read your suggestion and would like to shortly say that: The WBM should NOT be familiar (code-wise) with its users, and shouldn't include db.h in its implementation. It's a lower level infrastructure entity and as such, should be independent of its clients. It has been so far that way and should remain so.

While that used to be true, this PR (and others) is making it much less so. In "pure Rocks", the only relationship is thru the "StallInterface" which is generic. Speedb added a "WriteController" to the WBM. The WriteController is a class completely internal to a DB -- a user cannot create one and does not know what one does. The SessionID and Logger are public from the DB, but new to this class.

I am fine with extending the StallInterface to include more methods (GetLogger, etc) and I also prefer keeping the WBM pure, but do not believe this PR is keeping that purity.

Yuval-Ariel · 2023-06-25T06:19:24Z

Speedb added a "WriteController" to the WBM. The WriteController is a class completely internal to a DB -- a user cannot create one and does not know what one does

As part of #346 the WriteController is now a public class same as the WBM

re review

db/write_controller.cc

db/column_family.cc

reporting capabilities to the WriteController and the WriteBufferManager by saving the Loggers of the dbs which are using them internally and issuing WARN msgs to these Loggers whenever the state of the WC and WBM changes in regards to delaying.

Yuval-Ariel · 2023-09-27T13:08:26Z

@ayulas, i've fixed some unit tests that have failed.
in the process of fixing, i've changed a bit the registration part. can you take another look please?

ayulas · 2023-09-27T14:00:53Z

@Yuval-Ariel , you did a force push … so I don’t know what changes to do a review .. can you send me the only changes

Yuval-Ariel · 2023-09-27T14:07:56Z

these - https://github.com/speedb-io/speedb/compare/3fc9f6e9ce5998d222fdc99ac30b10aedfb66435..6ebe97480e0e65382ac7366aaabe9d69b11d2dcd

Add Logging capabilities to the WriteController and the WriteBufferManager by saving the Loggers of the dbs which are using them internally and issuing WARN msgs to these Loggers whenever the state of the WC and WBM changes in regards to delaying.

Yuval-Ariel requested a review from udi-speedb June 13, 2023 09:39

Yuval-Ariel self-assigned this Jun 13, 2023

Yuval-Ariel linked an issue Jun 13, 2023 that may be closed by this pull request

Add reporting to WBM and WC methods #453

Closed

udi-speedb closed this Jun 13, 2023

udi-speedb reopened this Jun 13, 2023

Yuval-Ariel force-pushed the add-reporting-to-wbm-and-wc branch 2 times, most recently from 52c9b8b to c21c4e4 Compare June 13, 2023 12:27

Yuval-Ariel requested review from mrambacher and removed request for udi-speedb June 19, 2023 06:08

mrambacher previously requested changes Jun 20, 2023

View reviewed changes

Yuval-Ariel requested a review from mrambacher June 22, 2023 06:18

Yuval-Ariel requested review from ayulas and removed request for mrambacher July 23, 2023 08:54

Yuval-Ariel force-pushed the add-reporting-to-wbm-and-wc branch from 91eb8f9 to f2990d0 Compare August 2, 2023 10:57

ayulas reviewed Aug 7, 2023

View reviewed changes

db/write_controller.cc Outdated Show resolved Hide resolved

db/column_family.cc Show resolved Hide resolved

Yuval-Ariel force-pushed the add-reporting-to-wbm-and-wc branch from f2990d0 to 4665b8a Compare August 17, 2023 07:49

Yuval-Ariel requested a review from ayulas September 21, 2023 09:50

ayulas approved these changes Sep 21, 2023

View reviewed changes

LOG reporting: Add Logging to WC and WBM

6ebe974

reporting capabilities to the WriteController and the WriteBufferManager by saving the Loggers of the dbs which are using them internally and issuing WARN msgs to these Loggers whenever the state of the WC and WBM changes in regards to delaying.

Yuval-Ariel force-pushed the add-reporting-to-wbm-and-wc branch from 3fc9f6e to 6ebe974 Compare September 27, 2023 12:37

Yuval-Ariel requested a review from ayulas September 27, 2023 12:38

ayulas approved these changes Sep 27, 2023

View reviewed changes

change logger_and_clientss to logger_and_clients

cfe9d2d

Yuval-Ariel merged commit c84e92a into main Oct 10, 2023
2 checks passed

Yuval-Ariel deleted the add-reporting-to-wbm-and-wc branch October 10, 2023 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logger: Add reporting capabilities to the WBM #556

Logger: Add reporting capabilities to the WBM #556

Yuval-Ariel commented Jun 13, 2023

Yuval-Ariel commented Jun 13, 2023

udi-speedb commented Jun 13, 2023

mrambacher left a comment

mrambacher Jun 19, 2023

Yuval-Ariel Jun 20, 2023

mrambacher Jun 19, 2023

Yuval-Ariel Jun 20, 2023

mrambacher Jun 20, 2023

Yuval-Ariel Jun 21, 2023

mrambacher Jun 21, 2023

Yuval-Ariel Jun 22, 2023

Yuval-Ariel commented Jun 20, 2023 •

edited

Loading

mrambacher commented Jun 21, 2023

Yuval-Ariel commented Jun 22, 2023

mrambacher commented Jun 23, 2023

udi-speedb commented Jun 23, 2023

mrambacher commented Jun 23, 2023

Yuval-Ariel commented Jun 25, 2023

Yuval-Ariel commented Sep 27, 2023

ayulas commented Sep 27, 2023 •

edited

Loading

Yuval-Ariel commented Sep 27, 2023

Logger: Add reporting capabilities to the WBM #556

Logger: Add reporting capabilities to the WBM #556

Conversation

Yuval-Ariel commented Jun 13, 2023

Yuval-Ariel commented Jun 13, 2023

udi-speedb commented Jun 13, 2023

mrambacher 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

Yuval-Ariel commented Jun 20, 2023 • edited Loading

mrambacher commented Jun 21, 2023

Yuval-Ariel commented Jun 22, 2023

mrambacher commented Jun 23, 2023

udi-speedb commented Jun 23, 2023

mrambacher commented Jun 23, 2023

Yuval-Ariel commented Jun 25, 2023

Yuval-Ariel commented Sep 27, 2023

ayulas commented Sep 27, 2023 • edited Loading

Yuval-Ariel commented Sep 27, 2023

Yuval-Ariel commented Jun 20, 2023 •

edited

Loading

ayulas commented Sep 27, 2023 •

edited

Loading