-
Notifications
You must be signed in to change notification settings - Fork 671
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
Feat/stackerdb event observer #3870
Conversation
…d a composite trait `RelayEventDispatcher`, and provide explicit upcasts to each constituent trait to preserve compatibility. Use this to implement relay logic for reporting stackerdb events from network results.
…rver to it'll get captured in NetworkResult (thereby exposing it to the Relayer thread)
Codecov Report
@@ Coverage Diff @@
## feat/stackerdb-rpc #3870 +/- ##
======================================================
- Coverage 0.16% 0.16% -0.01%
======================================================
Files 322 322
Lines 287351 287640 +289
======================================================
Hits 469 469
- Misses 286882 287171 +289
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…erver, not just chunk metadata
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.
LGTM -- just had a couple pretty superficial comments.
stackslib/src/net/stackerdb/mod.rs
Outdated
fn new_stackerdb_chunks( | ||
&self, | ||
contract_id: &QualifiedContractIdentifier, | ||
chunk_info: &[StackerDBChunkData], | ||
); |
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.
Since both callers of this method consume their StackerDBChunkData
instances anyways, this method could be consumptive, and then the event_dispatcher could avoid cloning this data:
fn new_stackerdb_chunks( | |
&self, | |
contract_id: &QualifiedContractIdentifier, | |
chunk_info: &[StackerDBChunkData], | |
); | |
fn new_stackerdb_chunks( | |
&self, | |
contract_id: QualifiedContractIdentifier, | |
chunk_info: Vec<StackerDBChunkData>, | |
); |
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.
Fixed
.enumerate() | ||
.filter(|(obs_id, _observer)| { | ||
self.stackerdb_observers_lookup.contains(&(*obs_id as u16)) | ||
|| self.any_event_observers_lookup.contains(&(*obs_id as u16)) |
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 inclined to say we shouldn't check the any_event_observers
-- it's a pretty unfortunate interface, because it means that node configs aren't forwards-compatible: if you've configured an event observer with *
, then the new endpoint is expected to be handled, but maybe you aren't handling it yet. I think the any_event_observer config option should be deprecated, but in the meantime, we shouldn't add new endpoints to it (I believe that this is how we've treated new event endpoints for a while).
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.
Fixed
…only forward stackerdb events to stackerdb-subscribed event observers
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Address #3856. Event observers can subscribe to stacker DB events by listing the "stackerdb" event concern in their configs, or the "any" concern.
StackerDBChunkEvent
JSON objects will get pushed to/stackerdb_chunks
for each successful chunk write (described indocs/event_observer.md
).