OADP-1067: Design for Batching VSM CRs #179
Conversation
openshift-ci
bot
added
the
approved
|
+1 that looks nice and clear to me. Thank you @eemcmullan! |
eemcmullan
requested review from
sseago and
kaovilai
and removed request for
sseago
docs/design/batching.md
Outdated
|
|
||
| If `dpa.spec.dataMover.concurrentBackupVolumes` or | ||
| `dpa.spec.dataMover.concurrentRestoreVolumes` is not set or it is set to zero, | ||
| then a default value will be used. This value is TBD. |
There was a problem hiding this comment.
Hmm. I'm wondering how this would work if a user wants to set it to zero, meaning "no limit". For example, if the default is 12, then a user setting this to 0 will result in the default of 12 being used. Maybe we don't allow "no limit" -- but practically speaking, the user can still have "no limit" by setting it to 1000 or something similarly huge.
There was a problem hiding this comment.
I guess one way to allow unlimited would be to implement the setting as an int pointer rather than an int -- that way we can distinguish between "not present; use default" (i.e. value is nil) and "unlimited" (value is 0; use this rather than default)" Thinking more about it, I'm leaning towards this. If we want "not specified" to mean "use the default" then we want the spec field to be *int64 rather than int64, so that 0 won't be overridden by the default.
There was a problem hiding this comment.
I agree, this approach sounds more reasonable. Will update with these details.
docs/design/batching.md
Outdated
| `volumeSnapshotRestores` if it is not set. Another option is to defing constants | ||
| such as `DefaultConcurrentBackupVolumes` and `DefaultConcurrentRestoreVolumes` | ||
| that can be set to a determined number, such as 0 or 12. More discussion is needed | ||
| around this topic. |
There was a problem hiding this comment.
If 0 means "unlimited" then we don't really need to know which option we're using in the design -- when we decide the default value, then it's just a matter of assigning the const in the golang file. If the default is zero, then it's unlimited -- if it's another number, then it's limited to that.
|
@sseago: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dymurray, eemcmullan, shubham-pampattiwar, sseago The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.