[fix][io] add protobuf ByteString to pulsar-io jdbc core #20259
Conversation
|
@bpereto Please add the following content to your PR description and select a checkbox: |
github-actions
bot
added
doc-not-needed
pulsar-io/jdbc/core/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.google.protobuf</groupId> | ||
| <artifactId>protobuf-java</artifactId> | ||
| <version>3.22.4</version> |
There was a problem hiding this comment.
Let's use the version in dependencyManagement in parent pom.
Also, cloud we use provided scope? (N.B. I don't take a considerate thought).
There was a problem hiding this comment.
+1 we need to use provided to avoid runtime clashes
Also get the already declared version
There was a problem hiding this comment.
I set the version to ${protobuf3.version} from pulsar/pom.xml and added the provided scope.
Is this what you ment by dependencyManagement ?
There was a problem hiding this comment.
@bpereto Just remove the <version> line. We already import:
<dependency>
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-bom</artifactId>
<version>${protobuf3.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>There was a problem hiding this comment.
@bpereto Also, please verify that provided statisfy your use case. One downside of provided is that - I'm not sure - if we don't pack protobuf into the NARball, it's possible to meet NoClassDefFoundError.
There was a problem hiding this comment.
I will test it with data, currently the sink is starting with the proposed changes.
Signed-off-by: tison <wander4096@gmail.com>
|
Perfect, currently the sink is starting, need to test further with data that will be sinked. |
|
works as intended for me. |
Codecov Report
@@ Coverage Diff @@
## master #20259 +/- ##
=============================================
+ Coverage 37.61% 72.95% +35.34%
- Complexity 12589 31962 +19373
=============================================
Files 1691 1868 +177
Lines 129028 138588 +9560
Branches 14066 15237 +1171
=============================================
+ Hits 48530 101107 +52577
+ Misses 74183 29444 -44739
- Partials 6315 8037 +1722
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Thanks for your verification! Merging... |
|
BTW, I notice another PR of yours #14584 that we may make progress if you review the comments and rebase on master. |
Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: tison <wander4096@gmail.com> (cherry picked from commit 849cbf3)
Fixes #20247
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: