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

add chunked/chunk size as setting/options in cli #8119

Merged
merged 1 commit into from
Mar 28, 2017

Conversation

corylanou
Copy link
Contributor

@corylanou corylanou commented Mar 10, 2017

While working on another PR, I noticed that chunking was hard coded in the CLI. This adds options to turn chunking on/off as well setting the chunk size.

This is very useful when doing any type of scripted querying or testing.

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
Required only if applicable

@corylanou corylanou changed the title add chunked/chunksize as setting/options add chunked/chunk size as setting/options in cli Mar 13, 2017
@@ -31,6 +31,10 @@ import (
// ErrBlankCommand is returned when a parsed command is empty.
var ErrBlankCommand = errors.New("empty input")

// DefaultChunkSize is the default number of results to return
// per response if chunked responses are turned on
const DefaultChunkSize = 10000
Copy link
Contributor

Choose a reason for hiding this comment

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

Default chunk size should just be set to the zero value. We don't want to duplicate the value from the server in the client. If the chunk size is set to zero, don't include it in the client request and that will cause the server to use the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, that makes sense. I see that the handler already sets to the default if we pass zero.

cmd = strings.ToLower(cmd)

// Remove the "chunk size" keyword if it exists
cmd = strings.TrimSpace(strings.Replace(cmd, "chunk size", "", -1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this check for chunk size at the beginning and trim it from the beginning? It seems to me like replace is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you are suggesting. Can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm saying just use strings.HasPrefix instead and then trimming the prefix instead of replacing anywhere within the line. The current implementation allows chunk size 4 chunk size.

@jwilder jwilder added this to the 1.3.0 milestone Mar 17, 2017
@corylanou
Copy link
Contributor Author

@jsternberg issues addressed. Please review again when you have a change. Thanks.

@corylanou
Copy link
Contributor Author

@jsternberg ping

@corylanou corylanou merged commit b4d61d4 into master Mar 28, 2017
@corylanou corylanou removed the review label Mar 28, 2017
@mark-rushakoff mark-rushakoff deleted the cjl-influx-chunked branch September 7, 2017 16:53
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.

5 participants