-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix task context fails with non string property value #16612
Conversation
@maytasm , this PR https://github.com/apache/druid/pull/16281/files also fixes the same issue. |
@@ -924,6 +924,11 @@ public CommandListBuilder addSystemProperty(String property, boolean value) | |||
return addSystemProperty(property, String.valueOf(value)); | |||
} | |||
|
|||
public CommandListBuilder addSystemProperty(String property, Object value) |
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 new method feels unnecessary.
Maybe we can use the approach used in #16281 instead.
Since the original author has not responded on that PR, we can proceed with this one.
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.
Done.
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java
Fixed
Show fixed
Hide fixed
SGTM. Closing |
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!
Fix task context fails with non string property value
Description
This is a regression caused by #14239
The above PR introduces a new method
addSystemProperty
with String type in the method definition for the property value. (Previously, we were usingStringUtils.format
which takes in Object as argument). The problem is thattask.getContextValue(propName)
has a return type which is a generic type parameter ContextValueType. This means the method can return a value of any type specified at the time of invocation. When the value of the context property is not a String, Java is still trying to call theaddSystemProperty
with String type causing the task to fail.i.e. if we have something like
we'll get the error:
This PR fix the issue by casting the return value from
task.getContextValue(propName)
to Object and then useaddSystemProperty
with Object type for the value parameter.This PR has: