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

Support for influxdb server version 0.9.3 #48

Closed
skynet opened this issue Aug 16, 2015 · 13 comments
Closed

Support for influxdb server version 0.9.3 #48

skynet opened this issue Aug 16, 2015 · 13 comments
Assignees

Comments

@skynet
Copy link

skynet commented Aug 16, 2015

There might be some breaking changes in 0.9.2, particularly with inserting integer values.

influxdata/influxdb#3606

Is the library updated to support these changes in 0.9.2 (and soon 0.9.3)?

@wdalmut
Copy link
Member

wdalmut commented Aug 19, 2015

Hi,

actually not, we probably work on those features in these days or weeks in order to be consistent with the influxdb changes.

In particular with:

#3519: --BREAKING CHANGE-- Update line protocol to require trailing i for field values that are integers
#3599: --BREAKING CHANGE-- Support multiple UDP inputs. Thanks @tpitale

We probably create a new branch in order to support InfluxDB < 0.9.3

https://github.com/influxdb/influxdb/blob/master/CHANGELOG.md#v092-2015-07-24

@wdalmut
Copy link
Member

wdalmut commented Aug 19, 2015

I just add the explaining issue from InfluxDB

influxdata/influxdb#3519

@wdalmut
Copy link
Member

wdalmut commented Aug 19, 2015

With actual versions all numbers will results as float64.

We probably adds a new configuration option that enable you to specify if you want to ensure integer data type during line protocol transformations.

Force integers detection

$options->setForceIntegers(true); //default as false
//...
$client->mark("serie", [
  "point" => 12, // Marked as int64
]);

Default behavior

$options->setForceIntegers(false); //default as false
//...
$client->mark("serie", [
  "point" => 12, // Marked as float64
]);

Anybody see any possible issue with this behavior?

@gianarb
Copy link
Contributor

gianarb commented Aug 19, 2015

In this moment we do not manage input values.. Input values for this library are transparent.
Maybe we can write few utilities about this feature..

Force integer

$client->mark("serie", [
  "point" => \InfluxDB\Filter\Integer::fromString("12"), // Marked as int64
]);

@wdalmut
Copy link
Member

wdalmut commented Aug 20, 2015

@gianarb interesting proposal, maybe with immutable objects:

$client->mark("serie", [
  "point" => new \InfluxDB\Filter\Integer("12"), // Marked as int64
]);

I don't know, i don't want to add too many components... I am thinking about type hinting in order to keep the library simple. Something like:

$options->setForceIntegers(false); //default as false
$client->mark("serie", [
  "point_as_float" => 12.0 // marked as float64
  "point_as_float2" => 12, // marked as float64
  "point_as_float3" => (int)14 // marked as float64
  "point_as_string" => "12", // marked as string
]);

$options->setForceIntegers(true); //default as false
$client->mark("serie", [
  "point_as_float" => 12.0 // marked as float64 (php double data type)
  "point_as_int" => 12, // marked as int64 (php int data type)
  "point_as_int2" => (int)"12", // marked as int64
  "point_as_string" => "12", // marked as string
]);

@gianarb
Copy link
Contributor

gianarb commented Aug 20, 2015

Yes I see your proposal.. I have no preference about them.
In my opinion setForceIntegers is the first method that works with input values.. I don't know if it's good task for a Client..

wdalmut added a commit that referenced this issue Aug 20, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.
@wdalmut wdalmut removed the BC Break label Aug 20, 2015
wdalmut added a commit that referenced this issue Aug 20, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.
@skynet
Copy link
Author

skynet commented Aug 20, 2015

https://github.com/influxdb/influxdb/releases/tag/v0.9.3-rc1 is out now. Do we have full support for it? Thanks.

@gianarb
Copy link
Contributor

gianarb commented Aug 20, 2015

@skynet what is your opinion about integer data type support? How do you wait to implement this feature in this library?? :)

Thanks

@skynet
Copy link
Author

skynet commented Aug 20, 2015

Transparently. And I wouldn't worry about breaking changes since new line protocol is going to be the next big step. Is there a fork that would not fail with 0.9.3, or a patch for master? Recently updated to 0.9.3 rc1 and I am not sure if I can still insert data.

@wdalmut
Copy link
Member

wdalmut commented Aug 21, 2015

You should be able to insert data with version 0.6.3 because changes in line protocol are not so big, the only issue should affects integer values that will be considered as float64 by InfluxDB in v0.9.3 where before those values are int64. Do you store integer values? Otherwise you can appy the patch:

https://patch-diff.githubusercontent.com/raw/corley/influxdb-php-sdk/pull/49.patch

And try with the PR #49

wdalmut added a commit that referenced this issue Aug 21, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.
@skynet
Copy link
Author

skynet commented Aug 21, 2015

So, the only difference could be in storage space used by InfluxDB? Otherwise I don't see how int64 vs. float64 would impact my queries. I was afraid that I won't be able to insert. Can apply the patch, although a new release would be better. Thank you!

wdalmut added a commit that referenced this issue Aug 22, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.
@wdalmut
Copy link
Member

wdalmut commented Aug 22, 2015

Not the storage space, influxdb cannot store multiple data value for the same column.

If you use

$client->mark("serie", [
  "valid" => true,
]);

And after

$client->mark("serie", [
  "valid" => 12.4,
]);

InfluxDB cannot store point 12.4 because the column valid is a boolean type column.

If you bring this situation to the int64 issue, if you mark the first point as

$client->mark("serie", [
  "cpu" => 12,
]);

This is marked as int64 in InfluxDB <=0.9.2 and then you cannot store float64 points

$client->mark("serie", [
  "cpu" => 12.4,
]);

This second value cannot be stored in place because the column cpu is an int64 column.

In InfluxDB 0.9.3 you can force the int64 data type using the trailing i, otherwise all numerical data are marked as float64.

Do you agree?

@wdalmut wdalmut changed the title Support for influxdb server version 0.9.2 Support for influxdb server version 0.9.3 Aug 22, 2015
wdalmut added a commit that referenced this issue Aug 23, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.

Steps:

 * Added configuration option `setForceIntegers`
 * Added data type doc
 * Updated influxdb configuration for version 0.9.3
 * Added force integers integration tests
 * Refactored message to line protocol
 * InfluxDB database version as env variable

**Refactored message to line protocol functions**

Actually the `message_to_line_protocol` function need the options set.
For that reason we have removed the `helpers.php` functions and moved
those utilities int the `AdapterAbstract` abstract base class.

**InfluxDB integration test environment variable**

In order to change the InfluxDB version we have prepared an env variable
in our Travis-CI configuration file `.travis.yml`
wdalmut added a commit that referenced this issue Aug 23, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.

Steps:

 * Added configuration option `setForceIntegers`
 * Added data type doc
 * Updated influxdb configuration for version 0.9.3
 * Added force integers integration tests
 * Refactored message to line protocol
 * InfluxDB database version as env variable

**Refactored message to line protocol functions**

Actually the `message_to_line_protocol` function need the options set.
For that reason we have removed the `helpers.php` functions and moved
those utilities int the `AdapterAbstract` abstract base class.

**InfluxDB integration test environment variable**

In order to change the InfluxDB version we have prepared an env variable
in our Travis-CI configuration file `.travis.yml`
wdalmut added a commit that referenced this issue Aug 23, 2015
As influxdb now support data type specification for integers, in order
to separate those values from float64, this is a very simple solution.

Related to issue #48.

Steps:

 * Added configuration option `setForceIntegers`
 * Added data type doc
 * Updated influxdb configuration for version 0.9.3
 * Added force integers integration tests
 * Refactored message to line protocol
 * InfluxDB database version as env variable

**Refactored message to line protocol functions**

Actually the `message_to_line_protocol` function need the options set.
For that reason we have removed the `helpers.php` functions and moved
those utilities int the `AdapterAbstract` abstract base class.

**InfluxDB integration test environment variable**

In order to change the InfluxDB version we have prepared an env variable
in our Travis-CI configuration file `.travis.yml`
@wdalmut wdalmut added new feature and removed WIP labels Aug 26, 2015
@wdalmut
Copy link
Member

wdalmut commented Aug 28, 2015

Resolved with PR #49 merged in the master branch and tagged as 0.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants