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 > > > > > > > > > > > > > > >
