-
Notifications
You must be signed in to change notification settings - Fork 476
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 possibility to reuse client as a core part of Reactive client #493
Conversation
* The InfluxDBImpl constructor has parameter for customise Retrofit.Builder. That builder is use in influxdb-java-reactive client for create reactive version of InfluxDBService. * The InfluxDBResultMapper is able to handle results which a different time precision * The InfluxDBResultMapper is able to map Integer to Instant
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
============================================
+ Coverage 87.53% 87.69% +0.15%
- Complexity 365 368 +3
============================================
Files 25 25
Lines 1500 1503 +3
Branches 167 167
============================================
+ Hits 1313 1318 +5
Misses 120 120
+ Partials 67 65 -2
Continue to review full report at Codecov.
|
Hi @bednar You are solving 3 Issues with one PR, from my understanding only your first point is related to rxjava. We only want to have PR´s which are focused on one single Issue. If i am wrong can you please elaborate more details. |
Hi @majst01, Yes, you are right. During implementation the reactive client I found two issues with
I will split the current PR to three pull requests:
It will be ok for you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were my comments that I incorrectly gave on the wrong PR from bonitoo and now moved to here.
@@ -146,7 +169,7 @@ public InfluxDBImpl(final String url, final String username, final String passwo | |||
break; | |||
} | |||
|
|||
this.retrofit = new Retrofit.Builder().baseUrl(url).client( | |||
this.retrofit = retrofitBuilder.baseUrl(url).client( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up: you may have here ConcurrentModificationException
as you are adding a converterFactory
to a shared and non thread-safe retrofitBuilder
. Worst case scenatio if you don't trigger a CME, you will end up having multiple converterFactory
added to the internal list in retrofitBuilder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. It could cause same errors as in #445, so I will use same approach to avoid potential problems:
Retrofit.Builder clonedRetrofitBuilder = retrofitBuilder.baseUrl(url).build().newBuilder();
this.retrofit = clonedRetrofitBuilder.client(clonedOkHttpBuilder.build())
.addConverterFactory(converterFactory).build();
@@ -152,7 +202,7 @@ void throwExceptionIfResultWithError(final QueryResult queryResult) { | |||
}); | |||
} | |||
|
|||
void cacheMeasurementClass(final Class<?>... classVarAgrs) { | |||
public void cacheMeasurementClass(final Class<?>... classVarAgrs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the access modifier from this method on this class and from all methods from other classes to the minimum required. There is no need to make this method public.
return ((Measurement) clazz.getAnnotation(Measurement.class)).name(); | ||
} | ||
|
||
public <T> ConcurrentMap<String, Field> getColNameAndFieldMap(final Class<T> clazz) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to have this method: you are not mocking it and now you are exposing an internal structure publicly to the external code, allowing it to be changed. Please, revert this change.
@@ -263,9 +324,11 @@ String getMeasurementName(final Class<?> clazz) { | |||
if (value instanceof String) { | |||
instant = Instant.from(ISO8601_FORMATTER.parse(String.valueOf(value))); | |||
} else if (value instanceof Long) { | |||
instant = Instant.ofEpochMilli((Long) value); | |||
instant = Instant.ofEpochMilli(toMillis((Long) value, precision)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please do a tinny improvement here by casting to primitives instead of Object wrappers? If you do, then please change the method signature from toMillis(final Long value, ...
to toMillis(final long value, ...
(just to avoid unboxing).
…ove unnecessary unboxing
@fmachado I made the required changes. Can you check it? |
Hi @bednar This PR still modifies around precision handling, and instantiation of a new influxdb service. I doubt these are all related to the usage in a reactive environment. As long as we have mixed modifications in this PR we will not accept that. |
Hi @majst01, thanks for response. Today, I will create separate PR for handling precision and leave this PR only for instantiation of a new InfluxDB service. |
@@ -126,7 +126,7 @@ public boolean isRetryWorth() { | |||
} | |||
} | |||
|
|||
private static InfluxDBException buildExceptionFromErrorMessage(final String errorMessage) { | |||
public static InfluxDBException buildExceptionFromErrorMessage(final String errorMessage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why public, if you want to test this like below, write the test in the matching package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a changelog.md entry with pointers where the usage of influxdb-java in a reactive environment is shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I want to reuse this method in influxdb-java-reactive to parse errors from HTTP header X-Influx-Error. If its is a problem I can use something like this:
String errorBody = String.format("{\"error\":\"%s\"}", errorHeader);
Can I let this method public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CHANGELOG.md was updated.
Hi @majst01, I changed method InfluxDBException#buildExceptionFromErrorMessage to private. Thanks for your review |
This PR solve #392 by adding possibility to reuse influxdb-java client as a core part of influxdb-java-reactive - Reactive Java client for InfluxDB.
Summary of changes
The
InfluxDBImpl
constructor has parameter for customiseRetrofit.Builder
. That builder is use in influxdb-java-reactive client for create reactive version ofInfluxDBService
.