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

Implement Issue #389 : Support for MessagePack #471

Merged
merged 6 commits into from
Jul 25, 2018

Conversation

lxhoan
Copy link
Contributor

@lxhoan lxhoan commented Jul 20, 2018

this implementation uses the messagepack-core only

@codecov-io
Copy link

codecov-io commented Jul 20, 2018

Codecov Report

Merging #471 into master will increase coverage by 0.17%.
The diff coverage is 90.55%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #471      +/-   ##
============================================
+ Coverage     86.91%   87.09%   +0.17%     
- Complexity      303      356      +53     
============================================
  Files            20       24       +4     
  Lines          1292     1495     +203     
  Branches        134      164      +30     
============================================
+ Hits           1123     1302     +179     
- Misses          111      127      +16     
- Partials         58       66       +8
Impacted Files Coverage Δ Complexity Δ
...uxdb/msgpack/MessagePackResponseBodyConverter.java 100% <100%> (ø) 2 <2> (?)
src/main/java/org/influxdb/InfluxDB.java 100% <100%> (ø) 0 <0> (ø) ⬇️
.../influxdb/msgpack/MessagePackConverterFactory.java 100% <100%> (ø) 3 <3> (?)
src/main/java/org/influxdb/InfluxDBFactory.java 93.33% <50%> (-6.67%) 5 <0> (ø)
...ava/org/influxdb/msgpack/MessagePackTraverser.java 87.05% <87.05%> (ø) 37 <37> (?)
...ava/org/influxdb/msgpack/QueryResultModelPath.java 90% <90%> (ø) 7 <7> (?)
src/main/java/org/influxdb/impl/InfluxDBImpl.java 89.01% <96.38%> (+0.18%) 73 <8> (+4) ⬆️
... and 2 more

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 0aac8f0...4eaa28a. Read the comment docs.

@lxhoan lxhoan requested review from majst01 and fmachado July 20, 2018 08:04
@lxhoan
Copy link
Contributor Author

lxhoan commented Jul 20, 2018

@majst01 , @fmachado
please help to take a look, thank you very much

@fmachado
Copy link
Contributor

fmachado commented Jul 20, 2018

@lxhoan nice work here! I'll take a look later.

@majst01
Copy link
Collaborator

majst01 commented Jul 20, 2018

@lxhoan i need some time as well.

@fmachado
Copy link
Contributor

@lxhoan I've nothing against reformatting the code but could you please avoid adding it to the same PR you are creating a new feature or fixing a bug? Thanks!

Copy link
Contributor

@fmachado fmachado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not find the UnitTest for the most imortant class in this PR: MessagePackTraverser responsible for creating the QueryResult and I think there are tests that should be removed (unless MessagePack support may have collateral effects on jitter/flush duration/etc.).

public QueryResult convert(final ResponseBody value) throws IOException {
try (InputStream is = value.byteStream()) {
MessagePackTraverser traverser = new MessagePackTraverser();
for (QueryResult queryResult : traverser.traverse(is)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't understand why you are traversing the InputStream but returning when the first element is retrieved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, there should be only one QueryResult so I return the first one. However this code can look ambiguous. I refactor MessagePackTraverser to expose more method for this logic. also add javadocs to explain

*/
@RunWith(JUnitPlatform.class)
@EnabledIfEnvironmentVariable(named = "INFLUXDB_VERSION", matches = "1\\.6|1\\.5|1\\.4")
public class MessagePackBatchOptionsTest extends BatchOptionsTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this class is necessary? How MessagePack support can affect jitter or how messages are flushed?

Copy link
Contributor Author

@lxhoan lxhoan Jul 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it does not. The reason to extends the tests for MessagePack is due to these 2 differences in MessagePack serializer (in comparison to JSON serializer) at server side :

  • Time in MessagePack is encoded as a 64 bit Long value (JSON as String)
  • Empty array is encoded as it is (JSON as empty)

Copy link
Collaborator

@majst01 majst01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lxhoan please first undo all formatting changes, we have way too much changes in this PR, which makes review more difficult.

this.adapter = adapter;

if (ResponseFormat.MSGPACK.equals(responseFormat)) {
String[] versionNumbers = version().split("\\.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go into its own method which can be then tested more easily.

}
if (httpUrl == null) {
throw new IllegalArgumentException("Unable to parse url: " + url);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert formatting changes as they make reviewing more difficult.

.bufferLimit(batchOptions.getBufferLimit())
.consistencyLevel(batchOptions.getConsistency())
.build();
this.batchProcessor = BatchProcessor.builder(this).actions(batchOptions.getActions())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, formatting with no changes in behavior.

this.batchEnabled.set(true);
return this;
}

@Override
public InfluxDB enableBatch(final int actions, final int flushDuration,
final TimeUnit flushDurationTimeUnit) {
public InfluxDB enableBatch(final int actions, final int flushDuration, final TimeUnit flushDurationTimeUnit) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

enableBatch(actions, flushDuration, flushDurationTimeUnit, threadFactory, (points, throwable) -> { });
public InfluxDB enableBatch(final int actions, final int flushDuration, final TimeUnit flushDurationTimeUnit,
final ThreadFactory threadFactory) {
enableBatch(actions, flushDuration, flushDurationTimeUnit, threadFactory, (points, throwable) -> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

}

/**
* {@inheritDoc}
*/
@Override
public List<String> describeDatabases() {
QueryResult result = execute(this.influxDBService.query(this.username,
this.password, SHOW_DATABASE_COMMAND_ENCODED));
QueryResult result = execute(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

call = this.influxDBService.postQuery(this.username,
this.password, query.getDatabase(), query.getCommandWithUrlEncoded(),
boundParameterQuery.getParameterJsonWithUrlEncoded());
BoundParameterQuery boundParameterQuery = (BoundParameterQuery) query;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

call = this.influxDBService.query(this.username,
this.password, query.getDatabase(), query.getCommandWithUrlEncoded());
}
if (query.requiresPost()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

if (datagramSocket != null && !datagramSocket.isClosed()) {
datagramSocket.close();
}
if (datagramSocket != null && !datagramSocket.isClosed()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

.append(duration)
.append(" REPLICATION ")
.append(replicationFactor);
queryBuilder.append(rpName).append("\" ON \"").append(database).append("\" DURATION ").append(duration)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting

@lxhoan lxhoan force-pushed the features/message-pack-8 branch 2 times, most recently from 403b741 to 54b5e8d Compare July 21, 2018 16:16
@lxhoan
Copy link
Contributor Author

lxhoan commented Jul 23, 2018

@majst01 , @fmachado
I fixed/answered your comments:

  • undo formatting
  • refactor + add javadocs to make MessagePackTraverser more unambiguous
  • unit test for MessagePackTraverser
  • checking of version support at querying time
    Please take a look

pom.xml Outdated
@@ -73,6 +73,9 @@
<exclude>docker-compose.yml</exclude>
</excludes>
</resource>
<resource>
<directory>src/test/resources</directory>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to include test resources in the artifact ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a mistake, I fixed it, tks for this finding

refactor + add javadocs to make MessagePackTraverser more unambiguous
MessagePackBatchOptionsTest simply extends BatchOptionsTest
checking of version support at querying time
fix test/resources was specified wrongly
Copy link
Contributor

@fmachado fmachado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MessagePackTraverserTest is not covering all branches in the traverse method and would be great to have a proper test showing that we can finally handle int/long/fp numbers correctly.
OK to merge.

@majst01 majst01 merged commit daef7e2 into influxdata:master Jul 25, 2018
@majst01
Copy link
Collaborator

majst01 commented Jul 25, 2018

@lxhoan can you please add a meaningful changelog entry, thanks

@lxhoan lxhoan deleted the features/message-pack-8 branch July 31, 2018 07:40
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.

4 participants