-
Notifications
You must be signed in to change notification settings - Fork 522
Stop logging indicators and drop them during migration #10521
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
Conversation
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
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.
batch = maybe_apply::<v0_0_2__to__v0_1_0::Migration>(&batch_version, batch); | ||
batch = maybe_apply::<v0_1_0__to__v0_1_1::Migration>(&batch_version, batch); |
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.
so is that an intra 0.24alpha port then? doesn't seem to hurt, indeed very nice to have these smaller, but I figure but maybe worth pointing out somewhere (unless I missunderstand)
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.
Yeah, we have substantial amount of data out there that still uses indicators but is on 0.1.0
. Without the bump we could not migrate it with our current logic.
use re_log_types::example_components::{MyLabel, MyPoints}; | ||
|
||
use super::*; | ||
|
||
struct DisplayDescrs(Chunk); |
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.
nice, cool construct for snapshot testing. Wasn't familiar with that pattern
0b68491
to
1500c7d
Compare
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/16119471982 |
### Related * Required for #10521. ### What This PR prevents static chunks _without components_ to enter the chunk store. > A static chunk without any components would be dangling from the start. > Our dangling logic retrieves relevant chunks by component descriptors, > so it would never be able to find it. So we never add that chunk in the > first place. Before this PR, the following code snippet would cause a `debug_assert!` to be hit. ```rs let chunk = Chunk::builder(entity_path.clone()) .with_component_batches(RowId::new(), TimePoint::STATIC, []) .build()?; let chunk = Arc::new(chunk); let events = store.insert_chunk(&chunk)?; ``` Also, that `chunk` would never get free'd up by our logic for finding dangling rows. #### More considerations Another option would be to emit both `Addition` and `Deletion` events in the same `insert`. Finally a last question would be do we want the chunk's entity path to show up in `ChunkStore::static_chunk_ids_per_entity`.
9ae497a
to
b222ccb
Compare
@rerun-bot full-check |
Started a full build: https://github.com/rerun-io/rerun/actions/runs/16121189121 |
Related
ChunkStore
#10530Important
This PR does not remove all occurrences of indicators from our code base, but I want to keep PRs small. Because of this I'm not closing #8129 yet.
What
This PR does several things, it:
rerun:is_indicator
) during migrationmultiple_scalars
snippetrerun rrd compare --ignore-chunks-without-components
option for snippet comparison.TODO