You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/04/17 12:48:43 UTC
[GitHub] [druid] bananaaggle opened a new pull request #11126: Add integration test for protobuf
bananaaggle opened a new pull request #11126:
URL: https://github.com/apache/druid/pull/11126
Now there is no stream integration test for Protobuf data. I add it to integration test.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-833170326
> > Hi. I can run one of those tests successful, but if when I run two tests such as 'protobuf/input_format' and 'protobuf/parser', it will fail. I check druid log and find no error. Can you help me check it? I use an absolute path in test file like 'file:///shared/wikiticker-it/wikipedia.desc'. Should I replace it as an alias?
>
> Sorry I haven't had a chance to get back to this until now. Hmm, this looks right to me, but I am not terribly familiar with this extension. I'll try to pull your branch today and see if I can help determine why this test is failing. If it proves too troublesome, I'm also ok if you want to split out adding the file based test into a separate PR, since the schema registry test is useful on its own.
>
Sorry, my expression is not very clear. When I test 'protobuf/input_format' and 'protobuf/parser' with schema registry, this trouble exists too. I don't know what happened because it works if run it alone. I think it's not very easy to find out bug. It easy to separate those file based test as another PR, but I want to figure out what cause this problem before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r637442762
##########
File path: integration-tests/docker/Dockerfile
##########
@@ -46,6 +47,9 @@ ADD lib/* /usr/local/druid/lib/
RUN wget -q "https://repo1.maven.org/maven2/mysql/mysql-connector-java/$MYSQL_VERSION/mysql-connector-java-$MYSQL_VERSION.jar" \
-O /usr/local/druid/lib/mysql-connector-java.jar
+RUN wget -q "https://packages.confluent.io/maven/io/confluent/kafka-protobuf-provider/$CONFLUENT_VERSION/kafka-protobuf-provider-$CONFLUENT_VERSION.jar" \
+ -O /usr/local/druid/lib/kafka-protobuf-provider.jar
+
Review comment:
good point, I did not realize that. Maybe a comment would be worthwhile
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r637112829
##########
File path: integration-tests/pom.xml
##########
@@ -466,6 +489,8 @@
<DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>${docker.run.skip}</DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>
<DRUID_INTEGRATION_TEST_INDEXER>${it.indexer}</DRUID_INTEGRATION_TEST_INDEXER>
<MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <CONFLUENT_VERSION>5.5.1</CONFLUENT_VERSION>
Review comment:
probably best to align the Confluent version with the Kafka version we use since Confluent dependencies often require Kafka dependencies. We still need to exclude those Kafka dependencies, but keeping them close should avoid compatibility issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle edited a comment on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle edited a comment on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-823117402
@clintropolis Is it there anything I can do when I finish this PR? I'm glad to contribute more code to community, any area is OK, such as core, ingestion or test. And where can I get more information about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-841651938
Hi, @clintropolis, I've solved problems. It caused by static DynamicMessage.Builder, I fixed it and all tests can pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-821906937
Thanks so much for taking this on, this fills one of the last test coverage gaps in the core ingestion extensions!
>Test with schema registry is OK, but it will include kafka-protobuf-provider in pom which is licensed by Confluent Community. Is it OK because is used only in test? If not, how can I do to add it to integration test?
I think we might be able to add the jar(s?) the same way we do for the MySQL connector, which is GPL licensed, by just downloading directly in the Dockerfile, see https://github.com/apache/druid/blob/master/integration-tests/docker/Dockerfile#L45.
>Another problem is file based protobuf encoder needs a file for test, I did't find out how to add it in code, so its test is not work actually. Can you give me some advices to solve them?
Hmm, I think a .proto file with the wikipedia schema could be converted into the .desc file needed for ingestion following the instructions here: https://druid.apache.org/docs/latest/development/extensions-core/protobuf.html#metricsdesc. Then, this descriptor file would need added to git, maybe in https://github.com/apache/druid/tree/master/integration-tests/docker or https://github.com/apache/druid/tree/master/integration-tests/docker/test-data. Next, it would need to be copied to the docker container as part of setup; sample wikipedia data for batch ingestion and some lookup configuration stuff are also copied in a similar manner, see https://github.com/apache/druid/blob/master/integration-tests/script/copy_resources.sh#L81. After this, wherever this file is copied to (path on the container) should now be available to specify in the ingestion spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-841760596
>Hi, @clintropolis, I've solved problems. It caused by static DynamicMessage.Builder, I fixed it and all tests can pass.
Glad you figured it out! The PR looks good to me, but [the spotbugs check is failing](https://travis-ci.com/github/apache/druid/jobs/505652794#L1427):
```
[ERROR] High: org.apache.druid.testing.utils.ProtobufEventSerializer.SCHEMA isn't final but should be [org.apache.druid.testing.utils.ProtobufEventSerializer] At ProtobufEventSerializer.java:[line 61] MS_SHOULD_BE_FINAL
```
I did manually trigger the kafka format integration tests and they passed, so after the spotbugs thing is fixed up I think it should be good to merge :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-842223516
> > Hi, @clintropolis, I've solved problems. It caused by static DynamicMessage.Builder, I fixed it and all tests can pass.
>
> Glad you figured it out! The PR looks good to me, but [the spotbugs check is failing](https://travis-ci.com/github/apache/druid/jobs/505652794#L1427):
>
> ```
> [ERROR] High: org.apache.druid.testing.utils.ProtobufEventSerializer.SCHEMA isn't final but should be [org.apache.druid.testing.utils.ProtobufEventSerializer] At ProtobufEventSerializer.java:[line 61] MS_SHOULD_BE_FINAL
> ```
>
> I did manually trigger the kafka format integration tests and they passed, so after the spotbugs thing is fixed up I think it should be good to merge 👍
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-841760596
>Hi, @clintropolis, I've solved problems. It caused by static DynamicMessage.Builder, I fixed it and all tests can pass.
Glad you figured it out! The PR looks good to me, but [the spotbugs check is failing](https://travis-ci.com/github/apache/druid/jobs/505652794#L1427):
```
[ERROR] High: org.apache.druid.testing.utils.ProtobufEventSerializer.SCHEMA isn't final but should be [org.apache.druid.testing.utils.ProtobufEventSerializer] At ProtobufEventSerializer.java:[line 61] MS_SHOULD_BE_FINAL
```
I did manually trigger the kafka format integration tests and they passed, so after the spotbugs thing is fixed up I think it should be good to merge :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-823112375
@clintropolis Hi. I can run one of those tests successful, but if when I run two tests such as 'protobuf/input_format' and 'protobuf/parser', it will fail. I check druid log and find no error. Can you help me check it? I use an absolute path in test file like 'file:///shared/wikiticker-it/wikipedia.desc'. Should I replace it as an alias?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r637117557
##########
File path: integration-tests/pom.xml
##########
@@ -366,6 +372,17 @@
</exclusion>
</exclusions>
</dependency>
+ <dependency>
+ <groupId>io.confluent</groupId>
+ <artifactId>kafka-protobuf-provider</artifactId>
+ <version>5.5.1</version>
+ <scope>provided</scope>
+ </dependency>
+ <dependency>
+ <groupId>com.google.protobuf</groupId>
+ <artifactId>protobuf-java</artifactId>
+ <version>3.11.0</version>
+ </dependency>
Review comment:
we can remove all this by enabling the protobuf extension, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r637148386
##########
File path: integration-tests/docker/Dockerfile
##########
@@ -46,6 +47,9 @@ ADD lib/* /usr/local/druid/lib/
RUN wget -q "https://repo1.maven.org/maven2/mysql/mysql-connector-java/$MYSQL_VERSION/mysql-connector-java-$MYSQL_VERSION.jar" \
-O /usr/local/druid/lib/mysql-connector-java.jar
+RUN wget -q "https://packages.confluent.io/maven/io/confluent/kafka-protobuf-provider/$CONFLUENT_VERSION/kafka-protobuf-provider-$CONFLUENT_VERSION.jar" \
+ -O /usr/local/druid/lib/kafka-protobuf-provider.jar
+
Review comment:
It doesn't actually include the dependency, there are licensing issues and that jar isn't solely Apache licensed, but actually dual Apache and Confluent community license, so people need to fetch it themselves similar to MySQL, see https://github.com/apache/druid/pull/10839#discussion_r570888190
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-823117402
@clintropolis It's there anything I can do when I finish this PR? I'm glad to contribute more code to community, any area is OK, such as core, ingestion or other tests. And where can I get more information about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #11126:
URL: https://github.com/apache/druid/pull/11126
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r637117341
##########
File path: integration-tests/pom.xml
##########
@@ -466,6 +489,8 @@
<DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>${docker.run.skip}</DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>
<DRUID_INTEGRATION_TEST_INDEXER>${it.indexer}</DRUID_INTEGRATION_TEST_INDEXER>
<MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <CONFLUENT_VERSION>5.5.1</CONFLUENT_VERSION>
Review comment:
we already define a confluent version here https://github.com/apache/druid/blob/master/extensions-core/protobuf-extensions/pom.xml#L37 which is newer, so we should at least align that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] xvrl commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r637116986
##########
File path: integration-tests/docker/Dockerfile
##########
@@ -46,6 +47,9 @@ ADD lib/* /usr/local/druid/lib/
RUN wget -q "https://repo1.maven.org/maven2/mysql/mysql-connector-java/$MYSQL_VERSION/mysql-connector-java-$MYSQL_VERSION.jar" \
-O /usr/local/druid/lib/mysql-connector-java.jar
+RUN wget -q "https://packages.confluent.io/maven/io/confluent/kafka-protobuf-provider/$CONFLUENT_VERSION/kafka-protobuf-provider-$CONFLUENT_VERSION.jar" \
+ -O /usr/local/druid/lib/kafka-protobuf-provider.jar
+
Review comment:
we should be able to avoid this by enabling the protobuf-extension which already includes this dependency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-821818927
@clintropolis Hi, happy weekend! I add integration test of Protobuf, but I meet some troubles. Test with schema registry is OK, but it will include kafka-protobuf-provider in pom which is licensed by Confluent Community. Is it OK because is used only in test? If not, how can I do to add it to integration test? Another problem is file based protobuf encoder needs a file for test, I did't find out how to add it in code, so its test is not work actually. Can you give me some advices to solve them? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r622634039
##########
File path: integration-tests/pom.xml
##########
@@ -466,6 +489,8 @@
<DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>${docker.run.skip}</DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>
<DRUID_INTEGRATION_TEST_INDEXER>${it.indexer}</DRUID_INTEGRATION_TEST_INDEXER>
<MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
Review comment:
nit: duplicate line
##########
File path: integration-tests/pom.xml
##########
@@ -466,6 +489,8 @@
<DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>${docker.run.skip}</DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>
<DRUID_INTEGRATION_TEST_INDEXER>${it.indexer}</DRUID_INTEGRATION_TEST_INDEXER>
<MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <CONFLUENT_VERSION>5.5.1</CONFLUENT_VERSION>
Review comment:
it might make sense to pull this up all the way to the top level `pom.xml`, then here, the avro extension, and the protobuf extension all share it to keep the version synced instead of each defining their own. I do notice that protobuf is using a newer version than avro and here, so I would be ok if we want to save this for a follow-up PR since it wouldn't be directly related adding this integration test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-842223516
> > Hi, @clintropolis, I've solved problems. It caused by static DynamicMessage.Builder, I fixed it and all tests can pass.
>
> Glad you figured it out! The PR looks good to me, but [the spotbugs check is failing](https://travis-ci.com/github/apache/druid/jobs/505652794#L1427):
>
> ```
> [ERROR] High: org.apache.druid.testing.utils.ProtobufEventSerializer.SCHEMA isn't final but should be [org.apache.druid.testing.utils.ProtobufEventSerializer] At ProtobufEventSerializer.java:[line 61] MS_SHOULD_BE_FINAL
> ```
>
> I did manually trigger the kafka format integration tests and they passed, so after the spotbugs thing is fixed up I think it should be good to merge 👍
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle commented on a change in pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle commented on a change in pull request #11126:
URL: https://github.com/apache/druid/pull/11126#discussion_r627025297
##########
File path: integration-tests/pom.xml
##########
@@ -466,6 +489,8 @@
<DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>${docker.run.skip}</DRUID_INTEGRATION_TEST_SKIP_RUN_DOCKER>
<DRUID_INTEGRATION_TEST_INDEXER>${it.indexer}</DRUID_INTEGRATION_TEST_INDEXER>
<MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <MYSQL_VERSION>${mysql.version}</MYSQL_VERSION>
+ <CONFLUENT_VERSION>5.5.1</CONFLUENT_VERSION>
Review comment:
Oh, 5.5.1 can support protobuf and in avro extension, its version is 5.5.1 too. In protobuf extension, its version is 6.0.1. I think those two versions are both OK because there is no change for core function. So which version is better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] bananaaggle edited a comment on pull request #11126: Add integration test for protobuf
Posted by GitBox <gi...@apache.org>.
bananaaggle edited a comment on pull request #11126:
URL: https://github.com/apache/druid/pull/11126#issuecomment-823117402
@clintropolis It's there anything I can do when I finish this PR? I'm glad to contribute more code to community, any area is OK, such as core, ingestion or test. And where can I get more information about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org