Conversation
barbarosalp
commented
Feb 14, 2019
barbarosalp
commented
- Fixes master branch failure because of ExampleTest
- Handling the error and fail the handler properly.
| import java.util.Map; | ||
|
|
||
| @RunWith(VertxUnitRunner.class) | ||
| @Ignore("This is an example test class.") |
There was a problem hiding this comment.
what is this example class test for ?
There was a problem hiding this comment.
I think this is just a reference about how to implement tests. Because there is no boostrap.servers defined, this causes master branch to fail.
| try { | ||
| partitionInfos = done.result(); | ||
| } catch (Exception e) { | ||
| handler.handle(Future.failedFuture(e)); |
There was a problem hiding this comment.
I have added a test for this. Can you guys have a look?
barbarosalp
force-pushed
the
handle-consumer-exception
branch
from
0f8044c to
d35cfa2
Compare
| for (org.apache.kafka.common.PartitionInfo kafkaPartitionInfo: done.result()) { | ||
| List<org.apache.kafka.common.PartitionInfo> partitionInfos = done.result(); | ||
| if (partitionInfos == null){ | ||
| partitionInfos = new ArrayList<>(); |
There was a problem hiding this comment.
Instead of returning an empty list of partitions, shouldn't we just fail the handler with a proper message saying, the topic does not exist and the auto-create topic is disabled
?
There was a problem hiding this comment.
I have pushed a new commit. Now, this fails when partitionInfos is null.
…o false for non-existing topic.
…andle-consumer-exception
|
Could you please have a look? @vietj @iranna90 @ppatierno |
|
@vietj @ppatierno can you please have a look at this? |
ppatierno
left a comment
ppatierno
left a comment
There was a problem hiding this comment.
There are a few comments and at same time conflicts to fix.
| @@ -0,0 +1,53 @@ | |||
| package io.vertx.kafka.client.common.impl; | |||
There was a problem hiding this comment.
We already have an Helper class, can you put these methods there please?
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; |
There was a problem hiding this comment.
all these changes are not needed, they are just moved imports. Please revert.
| import io.vertx.kafka.client.common.TopicPartition; | ||
| import io.vertx.kafka.client.common.impl.CloseHandler; | ||
| import io.vertx.kafka.client.common.impl.Helper; | ||
| import io.vertx.kafka.client.common.impl.PartitionsForHelper; |
| import io.vertx.kafka.client.consumer.KafkaConsumerRecords; | ||
| import io.vertx.kafka.client.consumer.KafkaReadStream; | ||
| import io.vertx.kafka.client.consumer.OffsetAndMetadata; | ||
| import io.vertx.kafka.client.consumer.OffsetAndTimestamp; |
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Properties; | ||
| import java.util.function.Supplier; |
There was a problem hiding this comment.
don't rearrange the imports please, revert this change
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.util.Properties; |
There was a problem hiding this comment.
don't rearrange the imports order please, revert.
| @@ -0,0 +1,68 @@ | |||
| package io.vertx.kafka.client.tests; | |||
There was a problem hiding this comment.
why we need a new test class for these tests?
|
@FrancescoBorzi @barbarosalp I reviewed the PR |
|
Thanks @ppatierno, I will try to address them soon. |
|
@barbarosalp still interested in it? |
6 participants
