Skip to content
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

Restore unique IDs for partition-local stores #1805

Merged
merged 4 commits into from
Aug 18, 2021

Conversation

lava
Copy link
Member

@lava lava commented Jul 28, 2021

📔 Description

Restore the ability to use offsets/ids when working with partition-local stores. This should also make it possible to use vast explore and vast get in combination with custom store backends.

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

Review commit-by-commit. Some points worth considering:

  • Should the query contain optional<ids> instead of ids, to distinguish between "no ids given" and "the empty set of ids requested"?

@mavam mavam changed the title Restore unique ids for partition-local stores Restore unique IDs for partition-local stores Jul 28, 2021
@tobim tobim added blocked Blocked by an (external) issue enhancement ✨ labels Jul 28, 2021
@mavam mavam removed the blocked Blocked by an (external) issue label Jul 30, 2021
@lava lava force-pushed the story/ch26808/partition-local-id-space branch 4 times, most recently from 38ce69c to 2b663bd Compare August 10, 2021 14:28
@lava lava marked this pull request as ready for review August 10, 2021 14:31
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

This seems to be in decent shape already, I think with a little effort we can get this over the line with only one more iteration.

There are still a few TODOs and FIXMEs in the code that should probably all get resolved too.

libvast/vast/system/meta_index.hpp Outdated Show resolved Hide resolved
libvast/vast/system/actors.hpp Outdated Show resolved Hide resolved
libvast/vast/system/actors.hpp Show resolved Hide resolved
libvast/test/system/meta_index.cpp Show resolved Hide resolved
@lava lava force-pushed the story/ch26808/partition-local-id-space branch 6 times, most recently from c98e79a to 7ce5a45 Compare August 18, 2021 09:41
 * Add offset/events fields into partition and
   partition synopsis flatbuffers.
 * Store info about ids in the meta index.
 * Add optional 'ids' field to query to query for specific
   ids.
Since the store already handles this case correctly, we can
simply delegate these kinds of queries forward.
@lava lava force-pushed the story/ch26808/partition-local-id-space branch from 7ce5a45 to 21416e4 Compare August 18, 2021 09:42
Copy link
Member

@tobim tobim left a comment

Choose a reason for hiding this comment

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

Pair review with @lava and verified that all integration tests pass with the local segment store now.

@lava lava enabled auto-merge August 18, 2021 11:19
@lava lava merged commit c529efe into master Aug 18, 2021
@lava lava deleted the story/ch26808/partition-local-id-space branch August 18, 2021 11:59
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