storageccl: accept and ignore a node-ID in the host of nodelocal:// uri #34797
Conversation
This file contains 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
This doesn't change the actual behavior of node-local storage -- it only allows (and then ignores) a nodeID to appear in the Host componant of the URI. Previously the host componant had to be empty. It still can be after this change or it can be a number. This paves the way to updating docs/usage/etc to show it used with a node-ID before changing it in a future version to _require_ the nodeID. Exact semantics of that change are TBD, but we wanted to get the change in that _allows_ the node-ID to be passed so that we could begin to show it used and start the multi-release process required for a breaking change. Release note (enterprise chage): nodelocal:// storage paths for BACKUP, RESTORE and IMPORT may include a node-ID in the Host part of the URI. It is not currently used (any node can get sent work and will look in its local IO directory) but will likely be required in the future.
|
This change is |
bdarnell
approved these changes
Feb 11, 2019
There was a problem hiding this comment.
Just to be clear, “host” in this context means the string between nodelocal:// and the next /, and we’re going to start allowing integer node IDs in that position? If so, sounds good to me.
Reviewed 4 of 4 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @mjibson)
|
Yes, currently you're allowed to do |
maddyblue
approved these changes
Feb 12, 2019
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Feb 12, 2019
34797: storageccl: accept and ignore a node-ID in the host of nodelocal:// uri r=dt a=dt This doesn't change the actual behavior of node-local storage -- it only allows (and then ignores) a nodeID to appear in the Host componant of the URI. Previously the host componant had to be empty. It still can be after this change or it can be a number. This paves the way to updating docs/usage/etc to show it used with a node-ID before changing it in a future version to _require_ the nodeID. Exact semantics of that change are TBD, but we wanted to get the change in that _allows_ the node-ID to be passed so that we could begin to show it used and start the multi-release process required for a breaking change. Release note (enterprise chage): nodelocal:// storage paths for BACKUP, RESTORE and IMPORT may include a node-ID in the Host part of the URI. It is not currently used (any node can get sent work and will look in its local IO directory) but will likely be required in the future. Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Build succeeded |
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.
This doesn't change the actual behavior of node-local storage -- it only allows (and then ignores) a nodeID to appear in the Host componant of the URI.
Previously the host componant had to be empty. It still can be after this change or it can be a number.
This paves the way to updating docs/usage/etc to show it used with a node-ID before changing it in a future version to require the nodeID.
Exact semantics of that change are TBD, but we wanted to get the change in that allows the node-ID to be passed so that we could begin to show it used and start the multi-release process required for a breaking change.
Release note (enterprise chage): nodelocal:// storage paths for BACKUP, RESTORE and IMPORT may include a node-ID in the Host part of the URI. It is not currently used (any node can get sent work and will look in its local IO directory) but will likely be required in the future.