imlement issue #439 : Retry capability for write(final BatchPoints batchPoints) as well

[lxhoan]

With this PR, if InfluxDBImpl is batch enabled, then write(final BatchPoints batchPoints) will use batch writer to write BatchPoints

this PR to make the consistency between writing single point and writing batch point, both must be retry capable as well

[codecov-io]

Codecov Report

Merging #495 into master will decrease coverage by 0.09%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master     #495     +/-   ##
===========================================
- Coverage     87.53%   87.44%   -0.1%     
- Complexity      365      367      +2     
===========================================
  Files            25       25             
  Lines          1500     1505      +5     
  Branches        167      167             
===========================================
+ Hits           1313     1316      +3     
- Misses          120      123      +3     
+ Partials         67       66      -1

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/org/influxdb/impl/BatchProcessor.java	98.47% <100%> (+0.01%)	18 <1> (+1)	⬆️
src/main/java/org/influxdb/impl/InfluxDBImpl.java	89.91% <100%> (+0.11%)	80 <2> (+2)	⬆️
...ava/org/influxdb/impl/RetryCapableBatchWriter.java	82.89% <100%> (ø)	17 <1> (ø)	⬇️
...ain/java/org/influxdb/impl/OneShotBatchWriter.java	100% <100%> (ø)	4 <1> (ø)	⬇️
src/main/java/org/influxdb/InfluxDBException.java	87.27% <0%> (-3.64%)	11% <0%> (-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 24c5542...b4aeaa3. Read the comment docs.

[lxhoan]

Hi @majst01 , @fmachado ,

Could you please take a look at this as well ?

Thanks

[fmachado]

I thought the whole purpose of offering write(final BatchPoints batchPoints) was to give the user the chance to use it's own batching implementation instead of the one provided by this library.

[majst01]

I second @fmachado this is kinda over engineered. If someone wants retry capabilities, only batching makes sense, so adding this kind of functionality is not worth the additional complexity.
I am not keen to take this.

[lxhoan]

I believe this PR will also increase the reliability/resilience of influxdb-java

retry for write(final BatchPoints batchPoints) is only enabled in case RetryCapableBatchWriter is used, otherwise it's still a direct writing

BTW, there not much additional complexity, much of the change is just code refactoring

[majst01]

But isn't that the point of having a seperate RetryCapableBatchWriter ? We introduce to much magic with this for the costs of more memory consumed. There might be, and i'm sure there are use cases, where this is not desired.

[lxhoan]

We introduce to much magic with this for the costs of more memory consumed

It's just easy to limit the retry buffer (used inside RetryCapableBatchWriter ), that is org.influxdb.BatchOptions.bufferLimit(int)

There might be, and i'm sure there are use cases, where this is not desired.

Yes, however it's also just easy to turn off the retry via BatchOptions.bufferLimit(int) as well

Moreover, I believe there are also use cases where user will expect writing of BatchPoints can be retried, especially when he see the batching can be backed off and retried

[fmachado]

I could not find an issue reported that would justify this implementation. Personally I would never use this implementation with batch enabled because, as I said before, for me the whole idea of using this method is to be able to write multiple points having on my side the control over the batching mechanism.

Anyway, this implementation caused some not nice collateral effects on the project. If you could find a better way to solve the issues I mentioned then I'm OK with this PR.

[fmachado]

I understand you don't want to expose this method but maybe we should review if this behavior is really necessary. I have some general points here (I hope I'm not being too picky):

• Because of this method you will be forced to declare the implementation (InfluxDBImpl) instead of the interface (InfluxDB) as you did on a few classes like OneShotBatchWriter line 12 for example. IMHO we have to chose if we are going to follow the API contract 100% of the time or just abandon it because this hybrid approach is not a good standard;
• The method name may cause some confusion. Check how the call stack when you use RetryCapableBatchWriter:

InfluxDBImpl.write(...)
   RetryCapableBatchWriter.write(...)
      RetryCapableBatchWriter.tryToWrite(...)
         InfluxDBImpl.writeNoRetry(...)

Unfortunately I don't have a better naming recommendation for it right now. I just want to point that you are calling a method writeNoRetry from a class that was supposed to be able to retry failed writes.

[lxhoan]

ok, I will open a new PR to extend InfluxDB interface, (one specific method for this retry purpose)

[majst01]

@lxhoan should we close this pr ?

[lxhoan]

closed since PR #503 is the final choice