Skip to content

feat: Refactor event processing code #2152

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sadhansood
Copy link
Contributor

@sadhansood sadhansood commented May 30, 2025

Description

We are looking to move event processing from running on individual storage node to its own dedicated centralized service. This service will be used by storage nodes to read events (and authenticate them) from instead of locally doing checkpoint processing. With that goal in mind, this is the first PR which moves event processing code from node and also refactors it for easier usage. NOTE: there is no functionality that is getting added or removed in this PR.

Test plan

Existing tests.


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that
a user might notice and any actions they must take to implement updates. (Add release notes after the colon for each item)

  • Storage node:
  • Aggregator:
  • Publisher:
  • CLI:

Copy link
Collaborator

@mlegner mlegner left a comment

Choose a reason for hiding this comment

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

Thank you so much for the reorganization. This generally LGTM. I have a few more detailed questions.

Note that there is an E2E test and some simtests that are failing currently.

I didn't really look at the moved code; my assumption is that it is essentially unchanged. Let me know if I should review something in a bit more detail.

@@ -16,6 +16,9 @@ pub use node::StorageNodeConfigLoader;
#[cfg(feature = "node")]
pub use node::errors::SyncNodeConfigError;

#[cfg(feature = "node")]
pub mod event;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: (Why) does this have to be pub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok it doesn't i think, will fix

};
let system_config = crate::node::events::event_processor::SystemConfig {
let processor_config =
crate::event::event_processor::config::EventProcessorRuntimeConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question (nit): Is there a reason to qualify this instead of importing as we do normally (also the SystemConfig below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok will fix

impl EventProcessor {
/// Creates a new event processor.
pub async fn new(
config: EventProcessorConfig,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Why was this changed from a reference to an owned parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah my bad, will fix

Copy link
Contributor

@liquid-helium liquid-helium left a comment

Choose a reason for hiding this comment

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

Thanks @sadhansood for the refactor, the tests are failing, otherwise, this looks good to me. I didn't dive into the code, assuming that they are no logic changes, focused on the structure changes.

Let me know if there are code pieces that I should pay more attention to.

@sadhansood
Copy link
Contributor Author

Thank you so much for the reorganization. This generally LGTM. I have a few more detailed questions.

Note that there is an E2E test and some simtests that are failing currently.

I didn't really look at the moved code; my assumption is that it is essentially unchanged. Let me know if I should review something in a bit more detail.

Thank tou Markus, I will look at the failing tests and fix them. There is no net new functionality, so it must be related to the refactor.

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.

3 participants