Allow to use BigInteger and BigDecimal for timestamps by J0s3f · Pull Request #657 · influxdata/influxdb-java

J0s3f · 2020-02-14T10:54:09Z

Allow to use BigInteger and BigDecimal for timestamps in the Point class to allow arbitrary timestamps with nanosecond precision.

Fixes #267

Won't break any existing code because it is still possible to use Long as timestamp.

…ass to allow arbitrary timestamps with nanosecond precision. Fixes #267

codecov-io · 2020-02-14T11:04:59Z

Codecov Report

Merging #657 into master will decrease coverage by 6.24%.
The diff coverage is 96.96%.

@@             Coverage Diff              @@
##             master     #657      +/-   ##
============================================
- Coverage     88.09%   81.85%   -6.25%     
+ Complexity      718      672      -46     
============================================
  Files            69       69              
  Lines          2487     2513      +26     
  Branches        263      268       +5     
============================================
- Hits           2191     2057     -134     
- Misses          208      370     +162     
+ Partials         88       86       -2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/influxdb/dto/Point.java	`93.36% <96.96%> (+0.33%)`	`53 <0> (+4)`	⬆️
...ava/org/influxdb/msgpack/QueryResultModelPath.java	`0% <0%> (-90%)`	`0% <0%> (-7%)`
...ava/org/influxdb/msgpack/MessagePackTraverser.java	`0% <0%> (-87.24%)`	`0% <0%> (-37%)`
...uxdb/msgpack/MessagePackResponseBodyConverter.java	`25% <0%> (-75%)`	`1% <0%> (-1%)`
src/main/java/org/influxdb/impl/InfluxDBImpl.java	`78.89% <0%> (-3.45%)`	`77% <0%> (-5%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45f6cf4...9f01975. Read the comment docs.

majst01 · 2020-02-14T11:15:11Z

-            this.time = TimeUnit.MILLISECONDS.convert(instant.toEpochMilli(), timeUint);
-            this.precision = timeUint;
+            TimeUnit timeUnit = tc.timeUnit();
+            if (timeUnit == TimeUnit.NANOSECONDS || timeUnit == TimeUnit.MICROSECONDS) {


I would leave MILLISECONDS as the default path.

But without this change it will never be possible to use more than millisecond precision?
The BigInteger is created only if nano- or microsecond precision is requested. The default for the annotation is still milliseconds.

majst01 · 2020-02-14T11:16:19Z

+    if (converterPrecision == null) {
+      converterPrecision = TimeUnit.NANOSECONDS;
+    }
+    if (this.time instanceof BigInteger) {


I am a bit afraid of the performance impact, this is in the hot path and instanceof is not known for beeing super performant. Can we prove somehow, how big the performance impact might be ?

I created a benchmark to measure the impact. It can be found here: https://github.com/J0s3f/influx-point-benchmark
To run it on you own machine, simply download the jar and run it via java -jar benchmarks.jar

Here are the results on my machine: https://gist.github.com/J0s3f/6e6feb1cef2644bdb060576e99b1e91f

I'd say there is no meaningful impact of this change.

Its about 4% slower ! Given that users complain about performance already, i dont want to take extra without double check if this is not possible in another way

If you take the numbers from my benchmark, benchmarkNewPoint is the changed code so it is 4% faster (but with bigger error margin) (more ops/s is better)

Cant image the extra code speeds up the processing ?

Benchmarks are just not accurate to that level on any PC that's running a full blown desktop OS.
I can try to increase the Forks in JMH but I don't have access to a bare metal machine I can use to run that for a few hours currently :(

And I changed one if in the existing code, this might have impact on the JIT compiler optimizations.

I made another benchmark run using the settings java -jar /benchmarks.jar -bm thrpt -f 5 -i 8 -si true -t max -wi 5 on a Quad-Core Intel Xeon VM that wasn't running anything else. (of course, I don't know what the host system was doing).

Here are the results: https://gist.github.com/J0s3f/379f9d2a1cfd6b77a5931b9422e80e42

OK, granted.

Can you please add some Unit tests to have these new logic covered as well: https://codecov.io/gh/influxdata/influxdb-java/compare/45f6cf44396acbf38161ab27b2c53ffbd2894fbf...fcc5a8fdc889d73af4a8495bad921db51c1dd33e/diff#D1-x558

…solution

… releases

majst01 · 2020-02-19T16:50:46Z

+     * @param precisionToSet the TimeUnit
+     * @return the Builder instance.
+     */
+    public Builder time(final Long timeToSet, final TimeUnit precisionToSet) {


Why is this needed, Long is a Number right.

Yes but in Java the Argument type is part of the method signature. For people always recompiling their code, it is not needed. But if someone would compile with a version without this pull request and than later just switch the dependency without recompiling, the program will fail with a NoSuchMethodError

I'm getting slow in Java, since years i switch to go for many reasons.
Is this ready to be merged now ?

Yes, I think it is complete. The code coverage is strange, but this must be some error. I just added this one method and suddenly some entirely different classes have zero coverage.
The coverage for my fork which is the exact same commit also shows correct coverage.

Allow to use BigInteger and BigDecimal for timestamps in the Point cl…

fcc5a8f

…ass to allow arbitrary timestamps with nanosecond precision. Fixes #267

majst01 reviewed Feb 14, 2020

View reviewed changes

Josef H.B. Schneider added 3 commits February 18, 2020 16:22

Add test for conversion of BitInteger/BigDecimal in Unit with less re…

289d0f0

…solution

updated comments

73efb96

Add method to add time as Long for binary compatibility with previous…

9f01975

… releases

majst01 reviewed Feb 19, 2020

View reviewed changes

majst01 merged commit 855708d into influxdata:master Feb 20, 2020

Conversation

J0s3f commented Feb 14, 2020

Uh oh!

codecov-io commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

J0s3f Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

J0s3f Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

J0s3f Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Feb 14, 2020 •

edited

Loading

J0s3f Feb 17, 2020 •

edited

Loading

J0s3f Feb 17, 2020 •

edited

Loading

J0s3f Feb 20, 2020 •

edited

Loading