You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/05/26 07:29:32 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
abhishekagarwal87 opened a new pull request #11302:
URL: https://github.com/apache/druid/pull/11302
We can get a classCastException in `org.apache.druid.server.scheduling.HiLoQueryLaningStrategy#computeLane` if the priority is set as string in query context
This PR has:
- [x] been self-reviewed.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11302:
URL: https://github.com/apache/druid/pull/11302#issuecomment-855786369
> ust minor nit comment, not important that you address, only if there are other changes requested by others.
>
> We might want to add an intelliJ inspection to ensure that we catch any future calls to `theQuery.getContextValue` and fail the build if they are found, to avoid this issue in the future.
@zachjsh if you have done this before, can you let me know how to do this? I am assuming we want to flag calls that where `theQuery.getContextValue` returns a non-string value. Or are you suggesting not to invoke this function at all even for string result?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11302:
URL: https://github.com/apache/druid/pull/11302#issuecomment-855788254
@zachjsh I am going to merge this PR and I can add the inspection in the next PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
Posted by GitBox <gi...@apache.org>.
kfaraz commented on pull request #11302:
URL: https://github.com/apache/druid/pull/11302#issuecomment-848824210
Is this a frequently occurring error?
Why would a user set the priority as a string?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 merged pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #11302:
URL: https://github.com/apache/druid/pull/11302
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #11302:
URL: https://github.com/apache/druid/pull/11302#issuecomment-848906949
> Is this a frequently occurring error?
> Why would a user set the priority as a string?
User is not setting the priority as String. I think below is what is happening
- all the entries in the file are being deserialized into `Properties` object. So even integer etc is being parsed as String.
- If that was not the case, such non-string entries will be filtered out in https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/guice/JsonConfigurator.java#L92 (properties.getPropertyValue returns null if value is not of string type)
Usually, the properties read will be translated into a Java object and any primitive types in the Java class will be parsed into numbers if required e.g. `SegmentLoaderConfig`. `DefaultQueryConfig` is special though since it has just a map `Map<String, Object>` so it's up to the caller to do the required parsing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #11302: Add integer parsing in HiLoQueryLaningStrategy
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #11302:
URL: https://github.com/apache/druid/pull/11302#discussion_r640858229
##########
File path: server/src/main/java/org/apache/druid/server/scheduling/HiLoQueryLaningStrategy.java
##########
@@ -67,8 +67,12 @@ public HiLoQueryLaningStrategy(
public <T> Optional<String> computeLane(QueryPlus<T> query, Set<SegmentServerSelector> segments)
{
final Query<T> theQuery = query.getQuery();
- // QueryContexts.getPriority gives a default, since we are setting priority
- final Integer priority = theQuery.getContextValue(QueryContexts.PRIORITY_KEY);
+ // QueryContexts.getPriority gives a default, but it can parse the value to integer. Before calling QueryContexts.getPriority
+ // we make sure that priority has been set.
+ Integer priority = null;
+ if (null != theQuery.getContextValue(QueryContexts.PRIORITY_KEY)) {
Review comment:
nit: Can be simplified to ternary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org