fix: fetch first-event-at from storage #1130
Open
+225
−34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 12975280436Details
💛 - Coveralls |
de-sh
reviewed
Jan 22, 2025
src/storage/object_storage.rs
Outdated
| @@ -747,3 +783,27 @@ pub fn ingestor_metadata_path(id: Option<&str>) -> RelativePathBuf { | |||
| &format!("ingestor.{}.json", INGESTOR_META.get_ingestor_id()), | |||
| ]) | |||
| } | |||
|
|
|||
| pub async fn get_stream_meta_from_storage( | |||
There was a problem hiding this comment.
Can we associate this with ObjectStore?
de-sh
reviewed
Jan 22, 2025
src/catalog/mod.rs
Outdated
| /// Ok(None) => println!("first-event-at not found"), | ||
| /// Err(e) => eprintln!("Error retrieving first-event-at from storage: {}", e), | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
The code comments aren't very helpful, can we restrict to only pointing out working and edge-cases?
de-sh
reviewed
Jan 22, 2025
src/catalog/mod.rs
Outdated
| /// Err(e) => eprintln!("Error retrieving first-event-at from storage: {}", e), | ||
| /// } | ||
| /// ``` | ||
| pub async fn get_first_event_from_storage( |
There was a problem hiding this comment.
Function should be associated to the storage object
de-sh
reviewed
Jan 22, 2025
| /// Err(e) => eprintln!("Error removing first-event-at from STREAM_INFO: {}", e), | ||
| /// } | ||
| /// ``` | ||
| pub fn reset_first_event_at(&self, stream_name: &str) -> Result<(), MetadataError> { |
There was a problem hiding this comment.
I feel a good comment would explain where we are using this function, e.g. when retention threshold is met and we are removing first event time since that information is outdated.
de-sh
reviewed
Jan 22, 2025
| /// None => eprintln!("Failed to update first-event-at"), | ||
| /// } | ||
| /// ``` | ||
| pub async fn update_first_event_at(stream_name: &str, first_event_at: &str) -> Option<String> { |
There was a problem hiding this comment.
Maybe we should pass DateTime/NaiveDateTime instead of using &str to represent this information?
current: query server fetches first-event-at from all the live ingestors but, this logic fails when ingestor which ingested the events for a stream is not reachable anymore, query server gets `None` change: fetch the first-event-at from all the stream jsons from storage find the earliest value, and update in server's memory map
nikhilsinhaparseable
force-pushed
the
first-event-at-fix
branch
from
988c060 to
b618c47
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
current: query server fetches first-event-at from all the live ingestors
but, this logic fails when ingestor which ingested the events for a stream is not reachable anymore, query server gets
Nonechange: fetch the first-event-at from all the stream jsons from storage,
find the earliest value, and update in server's memory map