You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/12/24 11:03:17 UTC
[GitHub] [pinot] SandishKumarHN opened a new pull request, #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
SandishKumarHN opened a new pull request, #10030:
URL: https://github.com/apache/pinot/pull/10030
pinot-protobuf plugin changes:
- This pull request introduces a new `feature` that allows users to specify the protobuf message java class name as a mandatory field, replacing the descriptorFile field. along with the existing option of protobuf message name and `.desc` file. it will be easier for users to pass a class name than generate descriptor files.
- Added new build descriptor methods to generate a descriptor using either the full qualified protobuf class name or a .desc file and message name. `buildDescriptor(String messageName, String descFilePathOpt)` and `buildDescriptorFromJavaClass(messageName)`
- Additionally, this PR removes the dependency on the third-party library `com.github.os72` for generating protobuf descriptor and renames all instances of `ProtoBuf*` to `Protobuf*`.
cc: @npawar @KKcorps
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] SandishKumarHN commented on pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
SandishKumarHN commented on PR #10030:
URL: https://github.com/apache/pinot/pull/10030#issuecomment-1376274647
> Thanks a lot for doing this! One request, can we avoid refactoring the class names such as `ProtoBufRecordReader` to `ProtobufRecordReader` for backward compatibility.
@KKcorps that makes sense, reverted changes, and review once again.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #10030:
URL: https://github.com/apache/pinot/pull/10030#issuecomment-1364515000
# [Codecov](https://codecov.io/gh/apache/pinot/pull/10030?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#10030](https://codecov.io/gh/apache/pinot/pull/10030?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (76d0ee0) into [master](https://codecov.io/gh/apache/pinot/commit/880a5c779fc441124ea14013a52d966885a58074?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (880a5c7) will **decrease** coverage by `50.67%`.
> The diff coverage is `71.81%`.
```diff
@@ Coverage Diff @@
## master #10030 +/- ##
=============================================
- Coverage 64.28% 13.61% -50.68%
+ Complexity 5669 176 -5493
=============================================
Files 1939 1939
Lines 105033 105083 +50
Branches 16041 16048 +7
=============================================
- Hits 67518 14303 -53215
- Misses 32631 89652 +57021
+ Partials 4884 1128 -3756
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `?` | |
| unittests2 | `13.61% <71.81%> (+0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/10030?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...not/plugin/inputformat/protobuf/ProtobufUtils.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9Qcm90b2J1ZlV0aWxzLmphdmE=) | `59.21% <59.21%> (ø)` | |
| [...ConfluentSchemaRegistryProtobufMessageDecoder.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9LYWZrYUNvbmZsdWVudFNjaGVtYVJlZ2lzdHJ5UHJvdG9idWZNZXNzYWdlRGVjb2Rlci5qYXZh) | `56.25% <100.00%> (ø)` | |
| [...plugin/inputformat/protobuf/ProtobufFieldInfo.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9Qcm90b2J1ZkZpZWxkSW5mby5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...n/inputformat/protobuf/ProtobufMessageDecoder.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9Qcm90b2J1Zk1lc3NhZ2VEZWNvZGVyLmphdmE=) | `80.00% <100.00%> (ø)` | |
| [.../inputformat/protobuf/ProtobufRecordExtractor.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9Qcm90b2J1ZlJlY29yZEV4dHJhY3Rvci5qYXZh) | `80.89% <100.00%> (ø)` | |
| [...gin/inputformat/protobuf/ProtobufRecordReader.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9Qcm90b2J1ZlJlY29yZFJlYWRlci5qYXZh) | `80.00% <100.00%> (ø)` | |
| [...putformat/protobuf/ProtobufRecordReaderConfig.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1pbnB1dC1mb3JtYXQvcGlub3QtcHJvdG9idWYvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9pbnB1dGZvcm1hdC9wcm90b2J1Zi9Qcm90b2J1ZlJlY29yZFJlYWRlckNvbmZpZy5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/spi/utils/LoopUtils.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvTG9vcFV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1310 more](https://codecov.io/gh/apache/pinot/pull/10030/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] SandishKumarHN commented on a diff in pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
SandishKumarHN commented on code in PR #10030:
URL: https://github.com/apache/pinot/pull/10030#discussion_r1065362676
##########
pinot-plugins/pinot-input-format/pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java:
##########
@@ -65,4 +75,114 @@ public static File createLocalFile(URI srcURI, File dstDir) {
+ "buffer descriptor {}", dstFile.getAbsolutePath(), srcURI);
return dstFile;
}
+
+ public static Descriptors.Descriptor buildDescriptor(String messageName, String descFilePathOpt) {
+ if (descFilePathOpt != null && !descFilePathOpt.isEmpty()) {
+ return getDescriptor(descFilePathOpt, messageName);
+ } else {
+ return buildDescriptorFromJavaClass(messageName);
+ }
+ }
+
+ /**
+ * Loads the given protobuf class and returns Protobuf descriptor for it.
+ */
+ private static Descriptors.Descriptor buildDescriptorFromJavaClass(String protobufClassName) {
+ Class<?> protobufClass;
+ try {
+ protobufClass = Class.forName(protobufClassName);
Review Comment:
@KKcorps yes, Isn't it the same with the descriptorFile option? If the import is done via Spark or Hadoop, users can pass the jar part of the —jars option.
This change eliminates the need to create a .desc file. The user would have a .proto jar rather than a .desc file. If you believe the descriptorFile-based option is useful, we can continue to provide users with both options.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] SandishKumarHN commented on a diff in pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
SandishKumarHN commented on code in PR #10030:
URL: https://github.com/apache/pinot/pull/10030#discussion_r1067176465
##########
pinot-plugins/pinot-input-format/pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java:
##########
@@ -65,4 +75,114 @@ public static File createLocalFile(URI srcURI, File dstDir) {
+ "buffer descriptor {}", dstFile.getAbsolutePath(), srcURI);
return dstFile;
}
+
+ public static Descriptors.Descriptor buildDescriptor(String messageName, String descFilePathOpt) {
+ if (descFilePathOpt != null && !descFilePathOpt.isEmpty()) {
+ return getDescriptor(descFilePathOpt, messageName);
+ } else {
+ return buildDescriptorFromJavaClass(messageName);
+ }
+ }
+
+ /**
+ * Loads the given protobuf class and returns Protobuf descriptor for it.
+ */
+ private static Descriptors.Descriptor buildDescriptorFromJavaClass(String protobufClassName) {
+ Class<?> protobufClass;
+ try {
+ protobufClass = Class.forName(protobufClassName);
Review Comment:
@KKcorps That makes sense; I hadn't considered the possibility of a server restart. will close 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on PR #10030:
URL: https://github.com/apache/pinot/pull/10030#issuecomment-1370368058
cc @snleee
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] SandishKumarHN closed pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
SandishKumarHN closed pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
URL: https://github.com/apache/pinot/pull/10030
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on a diff in pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #10030:
URL: https://github.com/apache/pinot/pull/10030#discussion_r1066662884
##########
pinot-plugins/pinot-input-format/pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java:
##########
@@ -65,4 +75,114 @@ public static File createLocalFile(URI srcURI, File dstDir) {
+ "buffer descriptor {}", dstFile.getAbsolutePath(), srcURI);
return dstFile;
}
+
+ public static Descriptors.Descriptor buildDescriptor(String messageName, String descFilePathOpt) {
+ if (descFilePathOpt != null && !descFilePathOpt.isEmpty()) {
+ return getDescriptor(descFilePathOpt, messageName);
+ } else {
+ return buildDescriptorFromJavaClass(messageName);
+ }
+ }
+
+ /**
+ * Loads the given protobuf class and returns Protobuf descriptor for it.
+ */
+ private static Descriptors.Descriptor buildDescriptorFromJavaClass(String protobufClassName) {
+ Class<?> protobufClass;
+ try {
+ protobufClass = Class.forName(protobufClassName);
Review Comment:
Using third-party jars is much more risky. Plus it would require a server restart. Descriptor files don't require server restart.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on a diff in pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #10030:
URL: https://github.com/apache/pinot/pull/10030#discussion_r1065352035
##########
pinot-plugins/pinot-input-format/pinot-protobuf/src/main/java/org/apache/pinot/plugin/inputformat/protobuf/ProtoBufUtils.java:
##########
@@ -65,4 +75,114 @@ public static File createLocalFile(URI srcURI, File dstDir) {
+ "buffer descriptor {}", dstFile.getAbsolutePath(), srcURI);
return dstFile;
}
+
+ public static Descriptors.Descriptor buildDescriptor(String messageName, String descFilePathOpt) {
+ if (descFilePathOpt != null && !descFilePathOpt.isEmpty()) {
+ return getDescriptor(descFilePathOpt, messageName);
+ } else {
+ return buildDescriptorFromJavaClass(messageName);
+ }
+ }
+
+ /**
+ * Loads the given protobuf class and returns Protobuf descriptor for it.
+ */
+ private static Descriptors.Descriptor buildDescriptorFromJavaClass(String protobufClassName) {
+ Class<?> protobufClass;
+ try {
+ protobufClass = Class.forName(protobufClassName);
Review Comment:
How is the user expected to upload class? e.g. if 30 pinot servers are running, users should copy their jar to all of them and then restart?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on pull request #10030: pinot-protobuf plugin generate descriptor using protobuf message java class name
Posted by GitBox <gi...@apache.org>.
KKcorps commented on PR #10030:
URL: https://github.com/apache/pinot/pull/10030#issuecomment-1375542806
Thanks a lot for doing this! One request, can we avoid refactoring the class names such as `ProtoBufRecordReader` to `ProtobufRecordReader` for backward compatibility.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org