-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
suppress headers in output for influx when they are the same #8122
Conversation
f625e8e
to
a322571
Compare
265b9a8
to
f46a9e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some suggested refactoring and the question about flush
cmd/influx/cli/cli.go
Outdated
@@ -777,38 +778,103 @@ func (c *CommandLine) writeJSON(response *client.Response, w io.Writer) { | |||
fmt.Fprintln(w, string(data)) | |||
} | |||
|
|||
func tagsEqual(prev, current map[string]string) bool { | |||
if prev == nil && current == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this check; nil
maps have zero length.
cmd/influx/cli/cli.go
Outdated
if prev == nil && current == nil { | ||
return true | ||
} | ||
if len(prev) != len(current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably just remove this too. I checked the source of DeepEqual
and one of the first things it does it check the length of two maps.
cmd/influx/cli/cli.go
Outdated
if prev == nil && current == nil { | ||
return true | ||
} | ||
if len(prev) != len(current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove redundant checks, as with tagsEqual
.
cmd/influx/cli/cli.go
Outdated
if prev.Name != current.Name { | ||
return false | ||
} | ||
if prev.Tags == nil && current.Tags == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this check.
cmd/influx/cli/cli.go
Outdated
return false | ||
} | ||
|
||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about you remove 807–812
and instead do:
return tagsEqual(prev.Tags, current.Tags) && columnsEqual(prev.Columns, current.Columns)
} | ||
csvw.Flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for flushing less often?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only need to call flush once. It's always streaming. And if you call flush to early, it actually messes up the alignment on the output (didn't read the source, but assuming it resets the column width tracking). There is no benefit to calling flush more than once regardless.
cmd/influx/cli/cli.go
Outdated
// if we are not at the end of the results, and we aren't suppressing headers, put in a new line | ||
// to break up the results | ||
// We don't print one if we are the first result, or the last result | ||
if !suppressHeaders && i > 0 && i < len(response.Results) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be ... && i < len(response.Results)-1 {
? The last result will have an index < len(response.Results)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is putting line returns between results, and it outputs it before the call to the headers. So the actual fix is to take out the check for i < len(response.Results)
as it is always true. Good catch.
} | ||
writer.Flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, flush moving down. I'm sure this is fine but I'm not sure on the reasoning behind it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
f46a9e2
to
d7696d1
Compare
@e-dard comments addressed. Take another gander when you get a chance. Thanks. |
also, this PR is based off of #8119 (chunked/size settings) to be able to test it out, so I'll rebase/merge once that one is merged. |
This might be more simple to implement by just starting to switch the CLI to using the alternative client. The alternative client already concatenates partial responses so you wouldn't have to check the headers. You can also just check to see if the statement id has changed or not now that the statement id gets returned with the response. I think it would be easier than checking the headers for equality. |
Moving to a new client is bigger change, and one I agree should be done, but this fixes a real problem now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me then. I'll prep a change in the next week for the new client.
83c3a8a
to
e07d845
Compare
d7696d1
to
9f674cc
Compare
If you encountered a chunked result while processing the results of a query, you would get the headers inserted for no reason. This PR suppresses the header when they are the same so as to not inflate the output. This is very useful when doing queries and grabbing a line count for adhoc testing, not to mention it created issues with csv layouts.
There is no good way to add testing for this without a larger refactor, so I've done extensive scenario adhoc testing. I'll list those below, but if you are reviewing this PR, the most important thing to look for is any scenarios I might of missed.
CSV Output
Before Change
After Change
Column Output
Before Change
After Change
Multiple Measurement + Chunked
Before Change
After Change
Required for all non-trivial PRs