Right now we're using PipelineResult as a sort of hybrid Future (e.g, waitUntilFinish is like Future.get()), and I think Stas has a reasonable point that this is confusing.
I know that figuring out the PipelineResult API for real is part of the pre-stable-release work -- sounds like the community should start gathering thoughts. JIRA is indeed probably a good place to do this -- there's probably alread a JIRA for "make PipelineResult a real API". Dan On Thu, Dec 22, 2016 at 3:39 PM, Stephen Sisk <[email protected]> wrote: > Thanks for speaking up that you found it confusing. Feedback is always > appreciated! > > For the State enum - I hear you that returning nulls can cause issues when > you're expecting an enum. > > We could in theory have the API return a new result object that includes > "Did the wait succeed?" + "What was the state?" Having said that, I suspect > you could then argue that waitUntilFinish would be cleaner if it just > returned a bool for whether or not it > > If you feel strongly about this, I'd suggest filing a JIRA ticket. A > breaking API change would have a higher bar (and this is a method that I > suspect has a wide audience, so a very high bar), but a 2nd copy of > waitUntilFinish/cancel() that just returned whether or not they > accomplished their goal might be interesting. I suspect now that I've > suggested a specific potential solution other folks might chime in as well. > :) > > S > > On Thu, Dec 22, 2016 at 11:34 AM Stas Levin <[email protected]> wrote: > > > Thanks for the detailed response. > > > > I did not have a particular scenario in mind, just found it a bit > confusing > > and was wondering if it was just me. > > > > Another thing that kind of threw me off was that waitUntilFinish() should > > return "null" (according to the documentation) in case it times out, > which > > IMHO is somewhat unexpected seeing that State is an enum. I guess the > > intent is to stress the fact that the returned "null" is the state of the > > *waiting* rather than the state of the underlying pipeline, yet it wasn't > > until the first couple of NullPointerExceptions that I realized this :) > > > > On Thu, Dec 22, 2016 at 9:05 PM Stephen Sisk <[email protected]> > > wrote: > > > > > Is there a particular scenario you're worried about here? I can see how > > > it's important to be consistent once in a terminal state, but I don't > see > > > how the API makes that worse. Since any of these methods retrieving > state > > > ultimately rely on the runner, if the runner is returning different > > values > > > over time after returning a terminal state, then it's all moot. > > > > > > To put it another way, let's say I have the following code: > > > pipeline.waitUntilFinish(); > > > State myState = pipeline.getState(); > > > State myState2 = pipeline.getState(); > > > // what guarantee is there that myState == myState2 ? > > > > > > Or let's say I have this: > > > pipeline.cancel(); > > > State myState = pipeline.getState(); > > > // if myState != cancel | fail, you will have angry tests > > > > > > It's noteworthy that waitUntilFinish and cancel result in terminal > > states, > > > where we shouldn't change to another state after we return from the > > > function. I'm not sure if that is documented/the consensus of the > > > community, but it's been how I think about those states. If a runner > were > > > changing states after being in a terminal state, then that can cause > > > issues. People like to write their tests/production code using > > > pipeline.waitUntilFinish() and then retrieving state, look at results, > > > etc... immediately after that. However, the fact that waitUntilFinish > > > returns a state doesn't cause that problem - it's that the runner impl > > > returns different values over time and that's true even if we only > return > > > state from getState. > > > > > > Is there a particular scenario driving your question? > > > > > > S > > > > > > > > > On Thu, Dec 22, 2016 at 10:29 AM Stas Levin <[email protected]> > wrote: > > > > > > > I see, thanks for the snippet. > > > > > > > > Won't the API be more robust (i.e. not leave it up to implementors' > > > > interpretation) by not exposing the state in any way other than > > > getState()? > > > > The snippet above will still work by mutating existing state, and in > > > > addition such an API will prevent returning any other (possibly > > > > inconsistent) state. > > > > > > > > So instead of doing: > > > > > > > > State myState = pipeline.waitUntilFinish(); > > > > State myOtherState = pipeline.getState(); > > > > > > > > // technically (myState != state) can be true > > > > > > > > Users will do: > > > > > > > > pipeline.waitUntilFinish(); > > > > State myState = pipeline.getState(); > > > > > > > > What do you think? > > > > > > > > > > > > On Thu, Dec 22, 2016 at 7:32 PM Lukasz Cwik <[email protected] > > > > > > wrote: > > > > > > > > > I think cancel and waitUntilFinish return state because they are > > > > > interacting with the runner and are likely to have pipeline state > > > > > information available to them at the time when performing that > > > operation. > > > > > > > > > > For example, waitUntilFinish(Duration) could just be a thin veneer > > of: > > > > > State state; > > > > > do { > > > > > state = getState(); > > > > > if (state is terminal) { > > > > > return state; > > > > > } > > > > > sleep > > > > > } while (time remaining) > > > > > return state; > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Dec 22, 2016 at 3:18 AM, Stas Levin <[email protected]> > > > wrote: > > > > > > > > > > > Hi all, > > > > > > > > > > > > I was wondering if the current API for PipelineResult might open > > the > > > > door > > > > > > to inconsistencies stemming from cancel() or waitUntilFinish() > > > > returning > > > > > > one state, and getState() returning another? Are such cases > legit? > > > > > > > > > > > > PipelineResult's API has a getState() method: > > > > > > > > > > > > State getState(); > > > > > > > > > > > > at the same time other methods such as cancel() and > > waitUntilFinish() > > > > > > return State as well: > > > > > > > > > > > > State waitUntilFinish(Duration duration); > > > > > > > > > > > > State cancel() throws IOException; > > > > > > > > > > > > Is this intentional? > > > > > > > > > > > > The alternative would be making cancel() and waitUntilFinish() > > return > > > > > void, > > > > > > so that the only (and thus consistent) way to obtain a > > > PipelineResult's > > > > > > state would be via getState(). > > > > > > > > > > > > Am I missing something? > > > > > > > > > > > > Regards, > > > > > > Stas > > > > > > > > > > > > > > > > > > > > >
