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 uint support into the write protocol #8835

Merged
merged 1 commit into from
Oct 9, 2017
Merged

Conversation

jsternberg
Copy link
Contributor

@jsternberg jsternberg commented Sep 15, 2017

  • Rebased/mergable
  • Tests pass

Copy link
Contributor

@rbetts rbetts left a comment

Choose a reason for hiding this comment

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

If this code is sufficient to parse incoming uint64 in wire protocol format, I'd like to have a flag to keep it disabled on master until we are ready to support uint64 through the whole stack. There are concerns that we will cause errors with Chrongraf / Kapacitor if we start storing and returning uint64 before we can move the change through the entire TICK stack.

if buf[i-1] != 'u' {
return i, ErrInvalidNumber
}
// Make sure the first char is not a '-' for unsigned
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is redundant with the initial check at line 833?

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 don't believe so. The check at line 833 prevents a person from writing m=-. This checks to make sure a negative wasn't used while looking at an unsigned integer.

@@ -2173,7 +2175,7 @@ m v=42i
m v="string"
m v=true
m v="string\"with\"escapes"
m v=42i,f=42,g=42.314
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be reading the test incompletely - but need to be sure we have tests for a negative unsigned int (error path), for max value, and for something greater than the max value.

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'll add those.

@jsternberg
Copy link
Contributor Author

I wasn't planning on having this merged until the query engine stuff was done, but I didn't think about the other applications. Should I build it as a configuration option or some compilation switch? I can also just block all incoming writes of unsigned with no compilation switch and no configuration option.

@jsternberg
Copy link
Contributor Author

It also may be difficult to have a switch that enables this. The section of code that does the parsing doesn't really have access to any configuration and catching it later than that would probably add some undesired overhead.

@phemmer
Copy link
Contributor

phemmer commented Sep 18, 2017

also is there any planned way to be able to change the type of an existing field?
I'm just imagining the scenario in telegraf where the various input plugins are storing their types as uint64, and the telegraf marshaller is sending them as signed integers. But once this PR is merged, and the telegraf marshaller is changed so that uint64 is actually sent as an unsigned integer, lots of writes will start failing because of conflicting field type.

@rbetts
Copy link
Contributor

rbetts commented Sep 19, 2017

@phemmer The ability to retroactively rewrite field types is not in scope. We are thinking through issues like the one you've raised but our immediate goal is to get to a functional uint64 field type.

@rbetts
Copy link
Contributor

rbetts commented Sep 19, 2017

@jsternberg I would avoid a configuration parameter - as this won't be long lived toggle once everything is done. A command line flag or environment variable might be preferable. I would like to get these bits landed in reasonable chunks on master.

@jsternberg
Copy link
Contributor Author

@rbetts done.

$ go install ./cmd/influxd
$ influx
> create database db0
> use db0
> insert cpu value=1u
ERR: {"error":"unable to parse 'cpu value=1u': invalid number"}

> exit
$ go install -tags uint ./cmd/influxd
$ influx
> use db0
> insert cpu value=1u
> exit

@jsternberg
Copy link
Contributor Author

I need to fix the tests since they fail since uint support is turned off. I'm also adding some additional parser tests for uint64 on top of the existing ones.

This is currently protected behind a conditional compilation flag. Use
`-tags uint` or `-tags uint64` to enable this.
@jsternberg jsternberg mentioned this pull request Oct 5, 2017
6 tasks
@jsternberg jsternberg merged commit e415d0b into master Oct 9, 2017
@jsternberg jsternberg deleted the js-uint-write-protocol branch October 9, 2017 15:55
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