Use defaults from meta package for CREATE DATABASE
#7119
Merged
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
jsternberg
force-pushed
the
js-create-database-use-defaults
branch
from
ca9fa4f to
61ae027
Compare
jsternberg
changed the title
meta package for CREATE DATABASEmeta package for CREATE DATABASE
jsternberg
force-pushed
the
js-create-database-use-defaults
branch
from
61ae027 to
457d460
Compare
| @@ -2812,3 +2806,11 @@ func mustParseDuration(s string) time.Duration { | |||
| } | |||
| return d | |||
| } | |||
|
|
|||
| func Duration(v time.Duration) *time.Duration { | |||
There was a problem hiding this comment.
Can you keep golint happy with some comments, or maybe even better move these to pkg or somewhere? Or alternatively make them unexported?
There was a problem hiding this comment.
I moved them to private functions although I think I'll move them to pkg in the future. I just didn't want to go through the effort right now.
jsternberg
force-pushed
the
js-create-database-use-defaults
branch
from
457d460 to
c0b7a0c
Compare
|
LGTM 👍 I think it would be worth getting at least two more review on this PR. @jwilder @corylanou @dgnorton |
jsternberg
force-pushed
the
js-create-database-use-defaults
branch
from
c0b7a0c to
19302f6
Compare
Instead of having the parser set the defaults, the command will set the defaults so that the constants for that are actually used. This way we can also identify which things the user provided and which ones we are filling with default values. This allows the meta client to be able to make smarter decisions when determining if the user requested a conflict or if the requested capabilities match with what is currently available. If you just say `CREATE DATABASE WITH NAME myrp`, the user doesn't really care what the duration of the retention policy is and just wants to use the default. Now, we can use that information to determine if an existing retention policy would conflict with what the user requested rather than returning an error if a default value ever gets changed since the meta client command can communicate intent more easily.
jsternberg
force-pushed
the
js-create-database-use-defaults
branch
from
19302f6 to
530b00b
Compare
|
👍 I would add that I would like to see more test coverage. Specifically around
|
timhallinflux
added a commit
that referenced
this pull request
Dec 20, 2016
removed duplicate entry in the bugfix section. #7119 was there 2x.
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.
Instead of having the parser set the defaults, the command will set the
defaults so that the constants for that are actually used. This way we
can also identify which things the user provided and which ones we are
filling with default values.