Skip to content
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

chore(mds): Refactor the master's masterconn #299

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from

Conversation

lgsilva3087
Copy link
Contributor

@lgsilva3087 lgsilva3087 commented Feb 13, 2025

This PR partially refactors masterconn.cc, which is used by Shadows and Metaloggers to represent a connection to the active Master server. The main changes include:

Key Improvements

  • Renamed masterconn to MasterConn for better readability.
  • Encapsulated responsibilities within MasterConn by making standalone functions class members, reducing the need for pointer calls.
  • Replaced C-style data structures with std::queue and std::vector, eliminating redundant implementations.
  • Prepared for multiple instances: While a singleton still exists, the new structure makes it easier to add more instances.
  • Reduced clang-tidy warnings, with more improvements to follow.
  • Moved code related to MetadataBackendFile to its appropriate file for better modularity.

Additional Enhancements

  • Improved readability by:
    • Adopting semi-camel-case variable names.
    • Renaming variables for better consistency and clarity.
  • Added documentation to improve maintainability.
  • Reduced enum sizes for efficiency.
  • Prepared for future separation of the struct into .h and .cc files, ensuring a cleaner distinction between interface and implementation.

Next Steps (Future PRs)

  • Keep refactoring beyond masterconn.cc, gradually improving related files.
  • Continue addressing clang-tidy warnings.
  • Remove magic constants related to legacy protocol packets, replacing them with the new serialization/deserialization mechanism.
  • Apply common project-wide improvements across all files.

This PR focuses solely on masterconn.cc to keep the changes manageable for review. More refinements will follow in subsequent PRs.

@lgsilva3087 lgsilva3087 self-assigned this Feb 13, 2025
@lgsilva3087 lgsilva3087 force-pushed the chore-mds-refactor-masterconn branch 2 times, most recently from 62376f9 to 72e9b22 Compare February 19, 2025 16:50
@lgsilva3087 lgsilva3087 force-pushed the chore-mds-refactor-masterconn branch 5 times, most recently from bf5e646 to b483a4d Compare February 25, 2025 10:59
Shadows and Metaloggers use the struct masterconn to represent a
connection to the active Master server in the current architecture.
This commit, partially refactors the masterconn.cc file. The main
changes are:

- Improve readability.
  - Semi camel case variable names.
  - Rename variables to be more consistent with their real usage.
- Add documentation.
- Reduce enums size.
- Remove some clang-tidy warnings (will continue).
- Prepare the struct to be split later into .h and .cc files, for
  better separation between interface and implementation.

The change was limited to only affect masterconn.cc to ease the review.
Next commits will continue improving the related files.

Signed-off-by: guillex <[email protected]>
The function masterconn_findlastlogversion does a very specific work
related to the metadata backend (metadata and changelogs). This commit
moves the responsibility to the correct place (MetadataBackendFile).

Signed-off-by: guillex <[email protected]>
The previous version was reimplementing a queue based on raw pointers
to outputHead and outputTail. This commit replaces the hand made queue
by a std::queue.

Signed-off-by: guillex <[email protected]>
This commit uses a std::vector to hold the packet buffer. The previous
version was using a raw uint_8 * approach. The new way allows to:

- Better readability.
- Avoid calling malloc and free directly.
- Reduce allocations in some scenarios (resize instead of free+malloc).
- Better RAII.

Signed-off-by: guillex <[email protected]>
The function masterconn_stats was never called, making unused also the
variables stats_bytesout and stats_bytesin. This commit removes the
function and the variables.

Signed-off-by: guillex <[email protected]>
The functions associated with the MasterConn struct are now member
funtions, removing the need to pass the pointer to the object every
time, and making easier to have multiple connections in the future.
The funcions were also renamed to be semi camel case.

The global functions were grouped and moved to the anonymous namespace.

Signed-off-by: guillex <[email protected]>
The following line was duplicated in the masterconn_init function:

eventloop_pollregister(masterconn_desc, masterconn_serve);

This commit removes the duplicated line.

Signed-off-by: guillex <[email protected]>
This commit introduces some constants in MasterConn to:

- Initialize all members (including cfg defaults).
- Remove magic constants in cfg loading.
- Remove clang-tidy warnings.

Signed-off-by: guillex <[email protected]>
Prevents the direct usage of new and delete.

Signed-off-by: guillex <[email protected]>
Most of the logging in MasterConn now uses the safs:: like style (C++),
instead of the legacy C-like functions (safs_pretty_syslog, etc.). The
change aims to improve the readability and the expresiveness of
spdlog + fmt.

The log messages that need 'errno' or 'silent' variants, were not
touched.

Signed-off-by: guillex <[email protected]>
Clang-tidy was complaining about some initializations of std::string at
compile time (may throw and can not be catched). This commit removes
the warnings by creating static inline functions returning the correct
values for Metalogger and Shadow.

The change uses static storage and const references to be as efficient
as the previous approach.

Signed-off-by: guillex <[email protected]>
@lgsilva3087 lgsilva3087 force-pushed the chore-mds-refactor-masterconn branch from 3e66373 to d01db60 Compare February 26, 2025 13:50
The file now aligns with the defined .clang-format style.

Signed-off-by: guillex <[email protected]>
@lgsilva3087 lgsilva3087 marked this pull request as ready for review February 26, 2025 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant