spec: default field sizes; clarify OPTIONAL fields #132
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
No changes for OPTIONAL error code fields since revisions for the error code section are WIP. |
jieyu
reviewed
Nov 1, 2017
spec.md
Outdated
| |------------|---------------------| | ||
| | 128 bytes | string | | ||
| | 4 KiB | map<string, string> | | ||
| | 4 KiB | repeated string | |
There was a problem hiding this comment.
As part of the change you did in #133, node_ids will be repeated string as well. If we limit this, we'll introduce a limit for the number of nodes this spec can handle. I'd suggest we remove this limitation here. Instead, explicitly mention that mount_flags has a limit of 4K bytes.
jdef
force-pushed
the
jdef_size_limits
branch
from
dd6510f to
37c8ba0
Compare
jdef
force-pushed
the
jdef_size_limits
branch
from
37c8ba0 to
70bd4a5
Compare
akutz
reviewed
Feb 2, 2018
| uint64 capacity_bytes = 1; | ||
|
|
||
| // Contains identity information for the created volume. This field is | ||
| // REQUIRED. The identity information will be used by the CO in | ||
| // subsequent calls to refer to the provisioned volume. This field | ||
| // should not exceed 1MiB. |
There was a problem hiding this comment.
Hi @jdef,
I just want to be clear that the ID string previously would be allowed a field size of 1MiB, but according to the spec today the volume ID is now limited to 128 bytes. Is that correct?
|
Yes. I forget where it was discussed (CSI WG sync, CSI Community sync,
both?) but the consensus was that 1MiB was probably overkill for an ID
field - that we should start small and if we find that it's too limiting we
can change the spec later to allow for longer IDs.
…On Fri, Feb 2, 2018 at 5:42 PM, Schley Andrew Kutz ***@***.*** > wrote:
***@***.**** commented on this pull request.
------------------------------
In spec.md
<#132 (comment)>
:
> uint64 capacity_bytes = 1;
// Contains identity information for the created volume. This field is
// REQUIRED. The identity information will be used by the CO in
- // subsequent calls to refer to the provisioned volume. This field
- // should not exceed 1MiB.
Hi @jdef <https://github.com/jdef>,
I just want to be clear that the ID string previously would be allowed a
field size of 1MiB, but according to the spec today the volume ID is now
limited to 128 bytes. Is that correct?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#132 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLCe1QWJy_VsY-7W0LuZWcbotK7u5ks5tQ49YgaJpZM4QN3iR>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in the CSI WG meeting on 2017-10-31, a "general field size" clause has been added to the spec. Also, language clarifying the intent and purpose of zero-values for OPTIONAL fields has been added to fields as appropriate.