-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[improve][io] Fix Kotlin version issue for functions built in Kotlin #19924
[improve][io] Fix Kotlin version issue for functions built in Kotlin #19924
Conversation
pom.xml
Outdated
@@ -1966,8 +1965,6 @@ flexible messaging model and an intuitive client API.</description> | |||
<test.additional.args/> | |||
<maven.compiler.source>8</maven.compiler.source> | |||
<maven.compiler.target>8</maven.compiler.target> | |||
<pulsar.broker.compiler.release></pulsar.broker.compiler.release> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be done intentionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhilashmandaliya to fix IDE warning, you can collapse the tag instead of removing it.
Anyway, keep one PR for one purpose (upgrade kotlin lib) can help you get a smooth review experience - we don't distract attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's an isolated version issue. Instead, you should take a look of how Pulsar uses okio and make an upgrade from the direct deps and then propagate to transitive deps.
cc @nicoloboschi since you're the last author for editing these versions in #13065. |
56625c8
to
8144bf9
Compare
cb3031d
to
104e44e
Compare
@nodece @tisonkun @lhotari @nicoloboschi |
pom.xml
Outdated
<okio.version>2.8.0</okio.version> | ||
<!-- override kotlin-stdlib used by okio in order to address CVE-2020-29582 --> | ||
<kotlin-stdlib.version>1.4.32</kotlin-stdlib.version> | ||
<okio.version>3.0.0</okio.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should move to 3.3.0, based on the changelog it's more stable and it upgrades kotlin to 1.8 as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoloboschi Latest stable version of okhttp3
4.10.0
uses 3.0.0
version of okio
. Is it okay to use 3.3.0
instead?
…t version of okio brings non-vulnerable kotlin dependency
@nicoloboschi PTAL at the updated library version change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@abhilashmandaliya license check failed:
The first lines can be redundant that you should remove. The final lines about kotlin-stdlib, I'm afraid, indicate an issue that we are missing to bundle them? |
@tisonkun With the latest okio library, we don't need to use a different version of Kotlin hence I removed it from the |
@abhilashmandaliya I get the error report wrong.
These lines mean that you're now (transitive) depending on these libs but they are not listed (mentioned) in the LICENSE file. You may double check if the version is correct (kotlin 1.5 and 1.6 while we plan to use 1.8? okio-jvm 3.0.0?). The LICENSE updating job I can do you a favor. But we should confirm the final status logically first. |
690c8e3
to
6306caa
Compare
The pr had no activity for 30 days, mark with Stale label. |
Signed-off-by: tison <wander4096@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I push a commit to suppress:
org.jetbrains.kotlin-kotlin-stdlib-1.8.20.jar mentioned in LICENSE, but not bundled
org.jetbrains.kotlin-kotlin-stdlib-common-1.8.20.jar mentioned in LICENSE, but not bundled
org.jetbrains.kotlin-kotlin-stdlib-jdk7-1.8.20.jar mentioned in LICENSE, but not bundled
org.jetbrains.kotlin-kotlin-stdlib-jdk8-1.8.20.jar mentioned in LICENSE, but not bundled
org.jetbrains-annotations-13.0.jar mentioned in LICENSE, but not bundled
But then the changeset seems a bit strange. Comment inline.
Also, you can ping the reviewers when you make progress - pushing new commits doesn't send notification.
</dependency> | ||
<dependency> | ||
<groupId>com.squareup.okhttp3</groupId> | ||
<artifactId>logging-interceptor</artifactId> | ||
<version>${okhttp3.version}</version> | ||
<exclusions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abhilashmandaliya if we exclude kotlin dep here, is it possible we are missing bundling several jars so that the function shade jar won't work?
Closing the PR as Pulsar 3.0.0 doesn't bring the older Kotlin version. Thanks a lot, @tisonkun for your help. |
Motivation
I was trying to implement a custom Sink connector using Kotlin. My Kotlin version was 1.8.10 and was using Jackson in my code. I saw a weird reflection-related error in my code. The debugging ended up with a finding that the Pulsar broker was bringing the Kotlin library dependency which was version 1.4.32 and that was getting loaded before my version. Hence my sink was using the older version of a specific class. While looking for Kotlin support in Pulsar code and feedback from the committers in this PR, I realized that Pulsar code is not using the Kotlin library directly but it is a transitive dependency. The latest stable version of the main dependency(okhttp3) bringing Kotlin(1.6.20) as a transitive dependency doesn't use the latest version of Kotlin(1.8.10). Hence, updated
okhttp3
,okio
, andKotlin
separately for the convenience of an existing structure.Putting the exception that I got here. (Not sure whether that is required):
Here
kotlin.Unit
class is loaded fromKotlin 1.4.32
whereas Jackson was compiled withKotlin 1.5.32
.Modifications
org.jetbrains.kotlin:kotlin-stdlib
library from1.4.32
to1.8.10
okhttp3
version from4.9.3
to4.10.0
okio
version from2.8.0
to3.0.0
Verifying this change
(Please pick either of the following options)
This change is a trivial rework.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: abhilashmandaliya#1