-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Pulsar SQL] Fix injection factory cast error (fix master broken) #9472
[Pulsar SQL] Fix injection factory cast error (fix master broken) #9472
Conversation
@@ -107,12 +107,6 @@ | |||
<version>${presto.version}</version> | |||
</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.
why is the pulsar client removed here? The pulsar presto connector need the admin client.
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.
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-client-admin-original</artifactId>
<version>${project.version}</version>
</dependency>
This dependency already exists, it contains pulsar-client-original
.
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.
awesome work
@@ -98,6 +99,7 @@ public String getTopicName() { | |||
} | |||
|
|||
private SchemaInfo loadSchema(BytesSchemaVersion bytesSchemaVersion) throws PulsarAdminException { | |||
Thread.currentThread().setContextClassLoader(InjectionManagerFactory.class.getClassLoader()); |
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.
if you set the Thread Context Classloader you have to restore it in a finally block
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.
Thanks for reminding me, I'll fix it.
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
SQL integration tests passed ! |
/pulsar-bot run-failure-tests |
Fixes #9470
Motivation
Fix master CI problem.
error log
Modifications
Verifying this change
This change is already covered by existing tests.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changes