-
Notifications
You must be signed in to change notification settings - Fork 50
Bring over changes from prestodb/drift #122
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
Conversation
Can you give the commit a proper title about what feature it's adding? |
import com.facebook.drift.protocol.TProtocolWriter; | ||
import jakarta.inject.Inject; | ||
|
||
import javax.inject.Inject; |
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.
nit: we can revert this. As part of the jetty upgrade, we migrated from javax.* to jakarta.*
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.
Since we downgraded guice, we actually still use javax.inject. You should see there are still usages of it throughout the codebase
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.
Oh, yes, forgot about that. So, we could use javax.inject
as well, but I think in this case we should use jakarta.inject
since this module is already using it in other classes.
drift/drift-codec-utils/pom.xml
Outdated
<optional>true</optional> | ||
</dependency> | ||
|
||
<dependency> |
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.
and revert this too when you remove the javax import.
@rschlussel can you please help merge this? |
Bring over changes from 1c51d99a0a629f0cd42804b5e31e1da95c3575d6 and 6c688e781fe13faac1ffefecf03b7d2b9fb4e479