-
Notifications
You must be signed in to change notification settings - Fork 6
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
Refactor Example Events Processor to use VersionTracker common step from SDK #8
Conversation
@@ -10,4 +10,4 @@ server_config: | |||
auth_token: "AUTH_TOKEN" | |||
request_name_header: "events-processor" | |||
db_config: | |||
postgres_connection_string: postgresql://postgres:@localhost:5432/example | |||
postgres_connection_string: postgresql://seanyang:@localhost:5432/example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oopsies
}; | ||
use async_trait::async_trait; | ||
use diesel::{upsert::excluded, ExpressionMethods}; | ||
pub struct ProcessorStatusSaverImpl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we name this DefaultProcessorStatusSaver
? also usually we leave out Impl
from the struct names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed as suggested. I also moved it to a new dir called common
, what do you think? Since its not specific to the events processor. I didn't name the dir common_steps
as its not actually a step... Alternately, I can create a factory-like method that returns a VersionTrackerStep with the appropriate ProcessorStatusSaver
so that we can keep the dir name common_steps
as the processor repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going with the latter suggestion as it prevents some duplicate code in the processor, too. That change will be in https://github.com/aptos-labs/aptos-indexer-processor-example/pull/6/files which has been rebased on top of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename file name to match with the struct name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to processor_status_saver
which will ultimately also have the BackfillProcessorStatusSaver
struct
)), | ||
Some(" WHERE processor_status.last_success_version <= EXCLUDED.last_success_version "), | ||
) | ||
.await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to map this to a ProcessorError?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
execute_with_better_error already returns a Result<usize, ProcessorError>
type. Is that sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing!
Purpose
Refactor the events processor to use the
VersionTracker
common step from SDK instead of implementing the entireLatestVersionProcessedTracker
(deleted) here. Implement the trait objectProcessorStatusSaver
to pass into the VersionTracker step.Technical Overview
Implement the new
ProcessorStatusSaver
from the SDK PR#63 to initialize the version tracker step.Testing
Ran locally: