Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RED-28784 documentation for ALTER, MGET and QUERYINDEX #103

Merged

Conversation

otnielvh
Copy link

No description provided.

docs/commands.md Outdated
@@ -259,6 +282,34 @@ TS.GET key
2) "23"
```

### TS.MGET
Multi-get all timeseries matching the specific filter.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should say "Get the last samples of timeseries matching the specific filter. " It's too confusing

Copy link
Author

@otnielvh otnielvh May 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is confusing and was trying to hint what MGET stands for. Anyway, your phrasing is much better, so I will use it.
Also, the GET command says "Get the last sample", to be consistent we can write: "Get the last samples matching the specific filter. "

docs/commands.md Show resolved Hide resolved
docs/commands.md Outdated
@@ -7,7 +7,7 @@
Create a new time-series.

```sql
TS.CREATE key [RETENTION retentionSecs] [LABELS field value..]
TS.CREATE key [RETENTION retentionSecs] [CHUNK_SIZE maxSamplesPerChunk] [LABELS field value..]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We initially thought not to expose chunksize in the docs. If we specify this, we need a default documentation below (as retenionSecs) and I also wonder if it's not possible to do so in TS.ADD and TS.INCRBY/DECRBY.
Given that we had more PR's on adding this to the docs, I suggest we add it, but than all the way. @gkorland @danni-m

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should remove CHUNK_SIZE from this PR, and after discussing add it in another PR.

Copy link
Collaborator

@K-Jo K-Jo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after this minor change good to go

docs/commands.md Outdated
@@ -17,6 +17,8 @@ Optional args:
* retentionSecs - Maximum age for samples compared to current time (in seconds)
* Default: The global retenion secs configuration of the database (by default, `0`)
* When set to 0, the series is not trimmed at all
* maxSamplesPerChunk - Maximum number of samples contained in each chunk.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one still needs to go

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching it, it is fixed now

@K-Jo K-Jo merged commit e5854f4 into RedisTimeSeries:master May 23, 2019
@otnielvh otnielvh deleted the otnielvh-mget-alter-queryindex-docs branch June 12, 2019 11: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.

None yet

2 participants