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

WIP: Enter frontier #198

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@frankmcsherry
Copy link
Member

commented Aug 12, 2019

This WIP PR proposes a new form of trace enter intended for regions, which allows the specification of a frontier, and which forcibly advances all times to that frontier (using Lattice::advance_by). The intent is that imported traces can then be entered into a region while advancing their frontier, to ensure that even for traces with non-trivially advanced frontiers one gets a consistent view of the times inside.

Alternately, we could write a more complicated import method, which seems like perhaps the right place to do this, but it seems like it would involve a larger amount of code and even more duplication (as import_core would need a new flavor for re-wrapping batches, unless we wanted to abandon non-importing imports).

cc @comnik

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

NB: One of the key features of this approach is that the wrapper should come without unnameable generic arguments, like closure parameters. This is meant to make it easier to use than enter_at, which allows an arbitrary closure (but then becomes hard to store because of the parameter).

@comnik

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

Thanks! Does this need some exercise before merging? I could try and replace the enter_at workarounds in 3DF with this.

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2019

I haven't tested this at all so while I'm totally up for you testing, you are welcome to do no such thing until I give it some minimal testing too. One thing I would love to get info on is the ergonomics, and whether this is the right place to do this vs somewhere else. For example, this could have been fused with enter, but is instead only a wrapper in the same scope, so you would need to have both an enter and a frontier wrapper, but that seemed better than forcing other people to introduce a region just to enter it.

@comnik

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I'll give it a try. Separate enter seems fine!

Can we add a corresponding enter_frontier on Arranged?

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Ah, do you mean like import_frontier? We could make that, and it feels like morally the right place to do it, but I am leery of a parallel implementation of import, which I think would be necessary as the produced batch types would need to be different. Maybe it can re-use lots of code, and it won't be so bad. I'm not certain, but will investigate!

@comnik

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

I thought something corresponding to

    pub fn enter_at<'a, TInner, F>(&self, child: &Child<'a, G, TInner>, logic: F)
        -> Arranged<Child<'a, G, TInner>, TraceEnterAt<Tr, TInner, F>>
        where
            Tr::Key: 'static,
            Tr::Val: 'static,
            Tr::R: 'static,
            G::Timestamp: Clone+Default+'static,
            TInner: Refines<G::Timestamp>+Lattice+Timestamp+Clone+Default+'static,
            F: Fn(&Tr::Key, &Tr::Val, &G::Timestamp)->TInner+'static,
    {
        let logic = Rc::new(logic);
        Arranged {
            trace: TraceEnterAt::make_from(self.trace.clone(), logic.clone()),
            stream: self.stream.enter(child).map(move |bw| BatchEnterAt::make_from(bw, logic.clone())),
        }
    }

s.t. something like arrangement.enter(scope).enter_frontier(&f) becomes possible?

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Oh, I mis-understood. We should definitely have that! :D

I think we might also want import_core_frontier to do something like this, and it seems like maybe it isn't so bad. Except a push in a bit.

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

In fact, I think it might be the case that if we have an import_XXX variant, there should probably (?) not be a need for the other variant (I think the only time one wants to advance by a frontier is on import, and it seems very reasonable to expect to do so as the only way we get sane observations).

@comnik

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Just tried it out. Things build, tests pass. It's looking to be a real ergonomics improvement. Really appreciate it!

See for yourself

@frankmcsherry

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

The diff does look pretty nice! There is a doctest that passes too, and I think that since this doesn't change any existing behavior it is probably pretty safe to land. I'll do that, but let me know if you experience any weirdness at all.

@frankmcsherry frankmcsherry merged commit 700601c into master Aug 14, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@comnik

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Will do! Thanks again.

@comnik comnik referenced this pull request Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.