Skip to content

Conversation

jseldess
Copy link
Contributor

Fixes #2180

@jseldess jseldess requested a review from a-robinson November 27, 2017 15:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

@a-robinson
Copy link
Contributor

:lgtm: but I'd wait for @mrtracy to chime in as well. In particular, I think we'll want to explain how the two different controls (sampling frequency and storage duration) interact with each other, and I'm guessing it's not obvious judging by the name of the storage duration cluster setting.


Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@jseldess jseldess requested a review from mrtracy November 27, 2017 15:44
@mrtracy
Copy link
Contributor

mrtracy commented Nov 28, 2017

I would not document the second method at all; changing that setting is not tested, and I personally do not think it is plumbed through the system correctly.

@mrtracy
Copy link
Contributor

mrtracy commented Nov 28, 2017

Opened cockroachdb/cockroach/issues/20310 to follow up on my concerns with this setting.

@jseldess
Copy link
Contributor Author

@mrtracy, I removed mention of that second method from the 1.2 docs. Should we also remove from the 1.1 and 1.0 docs? The cluster setting doesn't exist in those versions, so the env variable is the only way to affect timeseries data storage.

@cockroach-teamcity
Copy link
Member

@mrtracy
Copy link
Contributor

mrtracy commented Nov 29, 2017

:lgtm:, thanks Jesse


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jseldess
Copy link
Contributor Author

Merging despite the still broken libpqxx c++ driver site.

@jseldess jseldess merged commit 0aacff1 into master Nov 29, 2017
@jseldess jseldess deleted the timeseries-setting branch November 29, 2017 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants