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

influxdb-java-182 Allow write precision of TimeUnit other than Nanoseconds #321

Merged
merged 18 commits into from
May 11, 2018

Conversation

joelmarty
Copy link
Contributor

I worked on this a while ago, I was waiting for an answer before commiting so it can't be merged automatically, sorry about that!

Also I wrote some tests but since there's no public test environment I couldn't make them run.

@majst01
Copy link
Collaborator

majst01 commented May 23, 2017

This PR cant be merged because of conflicts, please first fix this.

@codecov-io
Copy link

codecov-io commented May 23, 2017

Codecov Report

Merging #321 into master will increase coverage by 0.43%.
The diff coverage is 87.09%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #321      +/-   ##
============================================
+ Coverage     86.04%   86.48%   +0.43%     
- Complexity      293      301       +8     
============================================
  Files            20       20              
  Lines          1261     1287      +26     
  Branches        133      134       +1     
============================================
+ Hits           1085     1113      +28     
+ Misses          116      114       -2     
  Partials         60       60
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/InfluxDB.java 100% <ø> (ø) 0 <0> (ø) ⬇️
src/main/java/org/influxdb/impl/InfluxDBImpl.java 87.34% <100%> (+1.06%) 67 <3> (+3) ⬆️
src/main/java/org/influxdb/dto/BatchPoints.java 72.34% <75%> (+3.29%) 22 <2> (+2) ⬆️
src/main/java/org/influxdb/dto/Point.java 83.52% <91.66%> (+0.59%) 34 <3> (+3) ⬆️

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 ce0188c...565c535. Read the comment docs.

@joelmarty
Copy link
Contributor Author

please verify the tests and process is correct before merging

@majst01
Copy link
Collaborator

majst01 commented May 25, 2017

for this single feature addition ~450 changed lines seems way too much.
There are also changes to unrelated lines

@joelmarty
Copy link
Contributor Author

450 lines of which half are the unit tests, but I also had trouble with the automatic code formatting (especially on the imports) and I didn't fix them all I guess.
I can understand it causes issues though, I will revert changes to unrelated lines.

@simon04
Copy link
Contributor

simon04 commented May 25, 2017

To me this looks like a very well done PR including numerous tests. 👍

*/
public String lineProtocol(final TimeUnit precision) {
final StringBuilder sb = new StringBuilder();
sb.append(KEY_ESCAPER.escape(this.measurement));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to synchronize this with #322
@simon04 can you keep track of this change in you PR, depending on what we merge first.

@majst01
Copy link
Collaborator

majst01 commented May 26, 2017

Yes this PR is in a shape we can merge, but i also want to wait for the next iteration. Once the actual version is published to maven central i will merge this as well.

@majst01 majst01 added this to the 2.7 milestone May 30, 2017
@majst01
Copy link
Collaborator

majst01 commented Jun 13, 2017

@joelmarty can you please fix the actual merge conflicts? i will merge then.

@majst01
Copy link
Collaborator

majst01 commented Jun 13, 2017

Thanks, but still build is failing, dont know exactly why.

@joelmarty
Copy link
Contributor Author

because it's a fucking nightmare to merge the changes now that a release has moved code all around. Might as well redo the feature from scratch.

@majst01
Copy link
Collaborator

majst01 commented Jun 14, 2017

I'm sorry for that, but i had a bunch of PR's for this sprint and either order of merge lead to conflicts of others.

@joelmarty
Copy link
Contributor Author

joelmarty commented Jun 14, 2017 via email

@majst01 majst01 modified the milestones: 2.7, 2.8 Jun 28, 2017
@majst01 majst01 modified the milestones: 2.8, 2.7 Jun 28, 2017
@bukajsytlos
Copy link

Hi there. Any news on this? We would like to get release 2.8 as soon as possible.
Thanks

@majst01 majst01 modified the milestones: 2.8, 2.9 Dec 7, 2017
@lxhoan
Copy link
Contributor

lxhoan commented May 11, 2018

@majst01 : I fixed the merge conflicts, please check and merge this PR
Thank you

/**
* @param precision the time precision to set for the batch points
*/
public void setPrecision(final TimeUnit precision) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this setter public, we have a builder which is responsible to do the object creation ?

Copy link
Contributor

Choose a reason for hiding this comment

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

well, seems this long-standing PR has not attracted its author anymore. Let me see if I can help something

@majst01
Copy link
Collaborator

majst01 commented May 11, 2018

OK, now im fine, @lxhoan can you add a changelog entry ?

@lxhoan
Copy link
Contributor

lxhoan commented May 11, 2018

change log added

@majst01 majst01 merged commit 5e66924 into influxdata:master May 11, 2018
@timhallinflux timhallinflux modified the milestones: 2.9, 2.11 May 11, 2018
@joelmarty joelmarty deleted the influxdb-java-182 branch May 18, 2018 13:38
@joelmarty
Copy link
Contributor Author

Thank you for taking care of this!

@lxhoan
Copy link
Contributor

lxhoan commented May 19, 2018

@joelmarty , you're welcome

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.

7 participants