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

Add a reducer server for performing aggregation operations. #221

Merged
merged 111 commits into from
Feb 28, 2024

Conversation

gibber9809
Copy link
Contributor

@gibber9809 gibber9809 commented Jan 8, 2024

Description

Adds a heavily refactored version of the reducer server from the prototype distributed setup as well as the code necessary to construct local in memory reduction pipelines.

Validation performed

  • Tested that a version of the reducer server from earlier in this PR's lifetime works correctly with the prototype clp package setup.
  • Tested that new networking code in this reducer server interacts correctly with a dummy python search scheduler implementation
  • Validated that the current version of the reducer works correctly end-to-end after integrating with current clp package

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An incomplete review but something to start with.

Besides the inline comments (in order of priority):

  • Add class docstrings.
  • Fix most (if not all) of the clang-tidy warnings (using the clang-tidy config I gave you).
  • Add method docstrings for non-trivial public methods.
  • Add method docstrings for non-trivial private methods.

components/core/src/clp/MySQLDB.cpp Outdated Show resolved Hide resolved
components/core/src/clp/MySQLDB.hpp Outdated Show resolved Hide resolved
components/core/src/clp/MySQLPreparedStatement.cpp Outdated Show resolved Hide resolved
components/core/src/clp/MySQLPreparedStatement.cpp Outdated Show resolved Hide resolved
components/core/src/reducer/CMakeLists.txt Show resolved Hide resolved
components/core/src/reducer/reducer_server.cpp Outdated Show resolved Hide resolved
components/core/src/reducer/reducer_server.cpp Outdated Show resolved Hide resolved
components/core/src/reducer/reducer_server.cpp Outdated Show resolved Hide resolved
components/core/src/reducer/reducer_server.hpp Outdated Show resolved Hide resolved
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished a pass on the code. Can you address the unresolved comments and then test everything?

components/core/src/reducer/ServerContext.cpp Show resolved Hide resolved
@gibber9809
Copy link
Contributor Author

Finished a pass on the code. Can you address the unresolved comments and then test everything?

Tested that everything works end to end after my most recent set of changes.


// Validate arguments
if (m_reducer_host.empty()) {
throw std::invalid_argument("reducer-host cannot be empty.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to catch the exceptions here or in main, right? Otherwise main will throw which is no bueno. My preference would be to catch them here like we do in the other programs.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit msg: Add a reducer server for performing aggregation operations. (#221)?

Dropped "distributed" since it might imply that the reducer itself is distributed, but no big deal if you want to keep it in.

@gibber9809 gibber9809 merged commit d90a18c into y-scope:main Feb 28, 2024
10 checks passed
@kirkrodrigues kirkrodrigues changed the title Add a reducer server that performs distributed aggregation operations Add a reducer server for performing aggregation operations. Feb 28, 2024
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.

2 participants