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

added consistency configuration for batch processing #385

Merged
merged 6 commits into from
Jan 8, 2018

Conversation

rbkasat
Copy link
Contributor

@rbkasat rbkasat commented Nov 21, 2017

No description provided.

@majst01
Copy link
Collaborator

majst01 commented Nov 21, 2017

Hi @rbkasat

You should start describing what problem you are facing and how you want to address this.

Beside that, a lot of checkstyle errors are produced, and therefore ci failed:

/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:31: error: Class BatchProcessor should be declared as final.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:121:39: error: Parameter consistencyLevel should be final.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:139:70: error: ',' is not followed by whitespace.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:200:80: error: ',' is not followed by whitespace.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:200:80: error: Parameter consistencyLevel should be final.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:208:26: error: '=' is not preceded with whitespace.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/BatchProcessor.java:208:27: error: '=' is not followed by whitespace.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/InfluxDBImpl.java:206: error: Line is longer than 120 characters (found 126).
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/InfluxDBImpl.java:206:99: error: ',' is not followed by whitespace.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/InfluxDBImpl.java:207:31: error: Parameter consistency should be final.
/home/travis/build/influxdata/influxdb-java/src/main/java/org/influxdb/impl/InfluxDBImpl.java:208: error: Line is longer than 120 characters (found 139).

Greetings

@rbkasat
Copy link
Contributor Author

rbkasat commented Nov 21, 2017

Hi @majst01 ,
Current implementation of BatchProcessor.java does not allow configuring consistency level for batch writes and defaults it to Consistency level one. We need to configure it to Consistency level "Any". This looks like a simple change so instead of creating issue I just went ahead and created a pull request with that change.
Let me know if you need more explanation on this.

@codecov-io
Copy link

codecov-io commented Nov 21, 2017

Codecov Report

Merging #385 into master will increase coverage by 0.34%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #385      +/-   ##
============================================
+ Coverage     83.12%   83.47%   +0.34%     
- Complexity      209      212       +3     
============================================
  Files            16       16              
  Lines           954      962       +8     
  Branches         96       96              
============================================
+ Hits            793      803      +10     
+ Misses          113      111       -2     
  Partials         48       48
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/influxdb/InfluxDB.java 100% <ø> (ø) 0 <0> (ø) ⬇️
src/main/java/org/influxdb/impl/InfluxDBImpl.java 84.42% <100%> (+0.92%) 57 <1> (+2) ⬆️
...rc/main/java/org/influxdb/impl/BatchProcessor.java 98.19% <100%> (+0.06%) 18 <1> (+1) ⬆️

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 df4af45...b9ac10a. Read the comment docs.

@fmachado
Copy link
Contributor

OK by me.
And by having another 'write' method, it reminds me that we should think about simplifying the API sooner than later. Maybe by moving parameters like consistencyLevel and retentionPolicy to the Point object.

@majst01
Copy link
Collaborator

majst01 commented Nov 22, 2017

Hi @rbkasat

you need to rebase you tests to match our latest migration tu junit5, after that i think this looks quite good.

@majst01
Copy link
Collaborator

majst01 commented Nov 23, 2017

@rbkasat you need to change this PR not to include all commits which where made before, i would like to see here only what implements the desired feature.

@timhallinflux
Copy link

The ability to change the consistency level is one part of the equation here -- and it should actually allow for ALL of the consistency parameters to be used (ANY, ALL, ONE, QUORUM).

The more complex part is to address what to do when the desired consistency is not met -- in the ALL case, the client may want to retry. Review:
https://docs.influxdata.com/enterprise_influxdb/v1.3/concepts/clustering/#write-consistency

@rbkasat
Copy link
Contributor Author

rbkasat commented Nov 25, 2017

@timhallinflux
Thanks for your suggestion and it definitely makes sense, but I think the library is easy to use the way it is currently. There is a way to configure callback with batch writes where a user can basically do a retry if he wants on the failed messages.
But if we want to implement retry, would probably involve more discussion and thinking on failure scenarios with a deep understanding of all failure messages.

I think setting consistency level is an immediate need in the batch writes and I would like to let the retry logic up to the user as a first step.

@fmachado fmachado merged commit 2c63292 into influxdata:master Jan 8, 2018
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.

5 participants