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

Enforce consolidated query results #71

Closed
comnik opened this issue Apr 30, 2019 · 7 comments
Closed

Enforce consolidated query results #71

comnik opened this issue Apr 30, 2019 · 7 comments

Comments

@comnik
Copy link
Owner

comnik commented Apr 30, 2019

...or extend Interest with an option? @eoxxs

@bachdavi
Copy link
Collaborator

bachdavi commented May 1, 2019

I am not completely sure about that.

By our assumption, clients are able to handle diffs, so no issue here.

The question is then: Is there a use case where a client might be interested in having unconsolidated results?

Seeing unconsolidated results may be surprising.

Extending Interest with an Option<bool> for consolidation and if None defaulting to use consolidate seems to be fine too.

@comnik
Copy link
Owner Author

comnik commented May 5, 2019

I just stumbled across Differential's ConsolidateStream that seems to be the sweet spot in that it treats consolidation as a nice-to-have, not a must-have, and thus doesn't require an arrangement. I think we can enforce that without needing to add a new option.

@frankmcsherry
Copy link
Collaborator

Just dropping in for a quick comment or two:

  1. Although Consolidate creates an arrangement, it shouldn't maintain the arrangement. That is, it drops the trace immediately, and so is only doing the batch formation, rather than the merging.
  2. ConsolidateStream is probably a sane thing to use, but .. it hasn't had a lot of love. It make a bunch of sense, what it should be doing and all, but yeah...

@comnik
Copy link
Owner Author

comnik commented May 5, 2019

So if I understand correctly, what Consolidate adds over ConsolidateStream is forwarding only fully formed batches while ConsolidateStream might be out of order?

@frankmcsherry
Copy link
Collaborator

ConsolidateStream could be incomplete, meaning that it forwards things that might cancel. It's def just a "best effort" consolidation, which could be useful for data reduction but which shouldn't be relied on for anything semantic.

@comnik
Copy link
Owner Author

comnik commented May 5, 2019

Ok that makes sense! Cool I think in that case it's nicer to just consolidate all results, in order to avoid sending incomplete results.

@comnik comnik closed this as completed in d27602c May 5, 2019
@bachdavi
Copy link
Collaborator

bachdavi commented May 5, 2019

Yeah that seems to make sense!

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

No branches or pull requests

3 participants