Implement Issue #389 : Support for MessagePack #462
Conversation
do not use Jackson annotation on QueryResult
Codecov Report
@@ Coverage Diff @@
## master #462 +/- ##
============================================
+ Coverage 86.63% 86.99% +0.36%
- Complexity 302 305 +3
============================================
Files 20 20
Lines 1294 1346 +52
Branches 135 139 +4
============================================
+ Hits 1121 1171 +50
- Misses 113 114 +1
- Partials 60 61 +1
Continue to review full report at Codecov.
|
| }); | ||
| messagePackFactory.setExtTypeCustomDesers(extTypeCustomDeserializers); | ||
|
|
||
| mapper = new ObjectMapper(messagePackFactory); |
There was a problem hiding this comment.
ObjectMapper are thread-safe and should be reused. I know we are not expecting to have users with 2^100 instances of this InfluxDB client but we are wasting resources here.
| final ObjectReader objectReader = mapper.readerFor(javaType); | ||
| chunkProccesor = new MessagePackChunkProccesor(objectReader); | ||
| } else { | ||
| mapper = new ObjectMapper(new MappingJsonFactory()); |
There was a problem hiding this comment.
Should an ObjectMapper instance exist even if msgpack is not being used?
| ResponseBody chunkedBody = response.body(); | ||
| chunkProccesor.process(chunkedBody, consumer); | ||
| } else { | ||
| //REVIEW: must be handled consistently with IOException. |
There was a problem hiding this comment.
If I'm not wrong, IOException will be thrown by Moshi when the stream consumer arrived at the end of the stream and it's not a real error (like errorBody != null).
There was a problem hiding this comment.
it's actually EOFException on reaching end of stream (handled in JSONChunkProccesor.process())
| * Accept MessagePack format (TRUE) or JSon (FALSE) for response from InfluxDB server | ||
| * @return a InfluxDB adapter suitable to access a InfluxDB. | ||
| */ | ||
| public static InfluxDB connect(final String url, final String username, final String password, |
There was a problem hiding this comment.
Can we think about a different approach on enabling MsgPack usage? I'm afraid we may end up by having many boolean parameters to activate/deactivate features (for example, we may add support to Protobuf).
+ reuse ObjectMapper, ObjectReader + Remove unused code + Respone format enum
|
I observed the query time reduced by ~20% for querying of 200K points and 2M points |
|
@lxhoan Good job! |
|
I even suppose that at server side, MessagePack serialization must be lighter than JSON serialization. So using MessagePack in query data can also reduce server side overheads |
|
I have done a heap analysis to see if there is any memory footprint difference between JSON and MessagePack queries. The experiment over the query of 2M points it's 13.8% better heap usage The reason is MessagePack encode time value of the Data Point as a Long value, and it's in general use pretty less memory than the time which formatted in String value (as in JSON) |
|
@lxhoan that's great! Could you please share with us how did you get those numbers? |
|
I use Eclipse Memory Analyzer (https://www.eclipse.org/mat) to analyze heap dump. I am going to write a document on it. Will copy you |
|
closed because PR #471 is the final PR |
In comparison to PR #460 , this PR:
(please refer to this comment in PR #460 for more details: #460 (comment))