Consider replacing SnapshotStatus enum with booleans

[saad-ali]

An enum when used for "state" encourages clients to write switch statements or or statements capturing every currently known state of enum and fail if the state is unrecognized. This leads to clients breaking when a new value is added to the enum (which should be a non-breaking additive change).

In Kubernetes we've moved to a model where we flaten the enum in to booleans, e.g. is_ready, which makes it harder for a client to be written in a forward incompatible way.

So request is to consider replacing SnapshotStatus enum with booleans.

[saad-ali]

@julian-hj made a good point: Kubernetes has to deal with many many consumers, this code is consumed by a handful of COs, so this is not as much of an issue.

Also @jdef mentioned that for terminal states, you would want the client to fail so optimizing for non-terminal states doesn't necessarily add much value.

Agreed with both points, will close this issue.

[saad-ali]

Also @jdef mentioned that for terminal states, you would want the client to fail so optimizing for non-terminal states doesn't necessarily add much value.

Discussed this some more with thockin. He pointed out that a good APIs should not make backwards compatibility breaking changes but may make additive changes. And when you have additive changes a list of booleans is much less likely to cause breakages then an enum (e.g. introducing a new state that new clients should handle, but doesn't invalidate any existing state or any existing clients handling of that state).

I'll leave this open for now. If we can't get consensus on it by the time we want to cut 1.0.rc we can abandon it. In the meantime assigning to @xing-yang @jingxu97 to explore.

[saad-ali]

Spoke with @jingxu97 @xing-yang and they pointed out that the multiple states are not necessary based on their implementation for Kubernetes. Error does not need to be a state.

Their proposal for simplifcation:

• When CreateSnapshotRequest is called, it blocks until the snapshot is cut, and returns a CreateSnapshotRequest with a new boolean isReadyToUse set to false indicating the snapshot has been taken and any workload can be resumed.
• CO then reissues CreateSnapshotRequest repeatedly until boolean isReadyToUse flips to true indicating the volume has been "processed" or "uploaded" and is ready to use to create new Snapshots.
• If there is an error during cutting the initial call will fail with gRPC error.
• If there is an error during "processing" (aka "uploading") the followup CreateSnapshotRequest will fail.
• There is no need to differentiate between a failure during cut vs upload because CO will not treat those differently.

[jdef]

At first glance I think that sounds OK to me.

…

On Wed, Nov 7, 2018 at 2:26 PM Saad Ali ***@***.***> wrote: Spoke with @jingxu97 <https://github.com/jingxu97> @xing-yang <https://github.com/xing-yang> and they pointed out that the multiple states are not necessary based on their implementation for Kubernetes. Error does not need to be a state. Their proposal for simplifcation: - When CreateSnapshotRequest is called, it blocks until the snapshot is cut, and returns a CreateSnapshotRequest with a new boolean isReadyToUse set to false indicating the snapshot has been taken and any workload can be resumed. - CO then reissues CreateSnapshotRequest repeatedly until boolean isReadyToUse flips to true indicating the volume has been "processed" or "uploaded" and is ready to use to create new Snapshots. - If there is an error during cutting the initial call will fail with gRPC error. - If there is an error during "processing" (aka "uploading") the followup CreateSnapshotRequest will fail. - There is no need to differentiate between a failure during cut vs upload because CO will not treat those differently. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#302 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLCo7LJYEFHGFSj7g3WPwpClAJZSIks5uszP6gaJpZM4YRRRz> .

[julian-hj]

Sounds good to me. It would bring the snapshot RPCs more in alignment with the other RPCs.