-
Notifications
You must be signed in to change notification settings - Fork 478
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
Implement Issue #389 : Support for MessagePack #460
Conversation
Codecov Report
@@ Coverage Diff @@
## master #460 +/- ##
============================================
+ Coverage 86.63% 86.81% +0.18%
- Complexity 302 309 +7
============================================
Files 20 20
Lines 1294 1327 +33
Branches 135 137 +2
============================================
+ Hits 1121 1152 +31
- Misses 113 115 +2
Partials 60 60
Continue to review full report at Codecov.
|
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.
Overall this PR does not look very clean, maybe our current implementation is not designed to be extendable as it should.
@@ -3,6 +3,8 @@ | |||
import java.util.List; | |||
import java.util.Map; | |||
|
|||
import com.fasterxml.jackson.annotation.JsonIgnoreProperties; |
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 do we introduce a new dependency to jackson ?
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 the converter https://github.com/komamitsu/retrofit-converter-msgpack (you proppsed it) depends on jackson
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.
OK, i didnt check that, my fault. But still, using another json converter with this change is not desired. We must think twice "
import com.fasterxml.jackson.databind.MappingIterator; | ||
import com.fasterxml.jackson.databind.MappingJsonFactory; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.fasterxml.jackson.databind.ObjectReader; |
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.
Same here, i really would like to prevent to switch from moshi to jackson with this PR. I am not against changing the json converter library, i just want to do it by "accident" when introducing messagepack format. We should do this move, once decided intentionally and without any other modifications.
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 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.
again, the convertor depends on Jackson
result = results.nextValue(); | ||
consumer.accept(result); | ||
} | ||
}; |
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.
Way too much code in the constructor.
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.
In a minor release, I would like to keep the implementation as back-ward compatible as possible. this constructor
InfluxDBImpl(final String url, final String username, final String password, final OkHttpClient.Builder client,
final InfluxDBService influxDBService, final JsonAdapter<QueryResult> adapter) {
in my opinion, even it seems awkward and used nowhere in the source code, we are not used if it is used by any user or not so I decided to keep it. That lead me to introduce the ChunkProcessor so we can have polymorphism for processing chunking query
@majst01, I believe Jackson has a better API on processing HTTP chunked response, You can compare 2 implementation of ChunkProccesor to see how different they are
@@ -130,7 +132,8 @@ public void testFlushDuration() throws InterruptedException { | |||
|
|||
//check no points writen to DB before the flush duration | |||
QueryResult result = influxDB.query(new Query("select * from weather", dbName)); | |||
Assertions.assertNull(result.getResults().get(0).getSeries()); | |||
List<Series> series = result.getResults().get(0).getSeries(); |
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.
Any reason why a test must be changed ?
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:
+in JSON response (for an empty query), the 'series' element does not exists
- in MessagePack response (for an empty query), series exist as an empty array
--> I think none of them are not wrong, just the consistency from server side. I am actually thinking to open an issue for the server
@majst01 , I think that's not a fault, because I found that this MessagePack retrofit converter is really documented in Retrofit Wiki https://github.com/square/retrofit/wiki/Converters). So I believe it is somewhat recommended by Retrofit guys. BTW, in a response to your comments. I propose following changes
What do you think ? |
closed because PR #471 is the final PR |
thi PR for Issue #389 : Support for MessagePack
I use the https://github.com/komamitsu/retrofit-converter-msgpack (thanks to @majst01)