You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "aishikbh (via GitHub)" <gi...@apache.org> on 2023/11/06 17:00:34 UTC
[PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
aishikbh opened a new pull request, #11955:
URL: https://github.com/apache/pinot/pull/11955
### Issue
If the taskName contains one or more '/' character in case of user generated tasks, the ZNode gets splits into a subdirectory resulting in the corresponding ZNode name not getting detected by Helix task executor.
Controller generated tasks don’t have this issue because controller has it's own logic to generate the taskName(table name + UUID) which makes sure that the taskName does not contain '/'.
### Solution
Addition of validation for taskName so that it does not contain the character '/'.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #11955:
URL: https://github.com/apache/pinot/pull/11955#discussion_r1384577699
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/task/AdhocTaskConfig.java:
##########
@@ -57,6 +57,7 @@ public AdhocTaskConfig(@JsonProperty(value = "taskType", required = true) String
@JsonProperty("taskConfigs") @Nullable Map<String, String> taskConfigs) {
Preconditions.checkArgument(taskType != null, "'taskType' must be configured");
Preconditions.checkArgument(tableName != null, "'tableName' must be configured");
+ Preconditions.checkArgument(taskName == null || !taskName.contains("/"), "'taskName' must not contain '/'");
Review Comment:
> @aishikbh Can we add the testing case to cover this? Please check `AdhocTaskConfigTest`
I have added tests related to invalid arguments in AdhocTaskConfig.
> @aishikbh - Could you add this to your message **taskName' must not contain path separator '/'**
I have rephrased the error message as well.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11955:
URL: https://github.com/apache/pinot/pull/11955#issuecomment-1795740305
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11955?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11955](https://app.codecov.io/gh/apache/pinot/pull/11955?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (1046316) into [master](https://app.codecov.io/gh/apache/pinot/commit/6638d4edcbf61a8078f4a1c2e4be36cc428cc239?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6638d4e) will **decrease** coverage by `26.64%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11955 +/- ##
=============================================
- Coverage 61.29% 34.66% -26.64%
+ Complexity 1140 6 -1134
=============================================
Files 2385 2309 -76
Lines 129071 125326 -3745
Branches 19955 19398 -557
=============================================
- Hits 79120 43439 -35681
- Misses 44220 78826 +34606
+ Partials 5731 3061 -2670
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-27.61%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.44% <0.00%> (-14.85%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-61.28%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.44% <0.00%> (-14.83%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.66% <0.00%> (-26.64%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.44% <0.00%> (-14.85%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.44% <0.00%> (-0.04%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11955/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11955?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../apache/pinot/spi/config/task/AdhocTaskConfig.java](https://app.codecov.io/gh/apache/pinot/pull/11955?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3Rhc2svQWRob2NUYXNrQ29uZmlnLmphdmE=) | `76.92% <0.00%> (-6.42%)` | :arrow_down: |
... and [830 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11955/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
Posted by "swaminathanmanish (via GitHub)" <gi...@apache.org>.
swaminathanmanish commented on code in PR #11955:
URL: https://github.com/apache/pinot/pull/11955#discussion_r1383822916
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/task/AdhocTaskConfig.java:
##########
@@ -57,6 +57,7 @@ public AdhocTaskConfig(@JsonProperty(value = "taskType", required = true) String
@JsonProperty("taskConfigs") @Nullable Map<String, String> taskConfigs) {
Preconditions.checkArgument(taskType != null, "'taskType' must be configured");
Preconditions.checkArgument(tableName != null, "'tableName' must be configured");
+ Preconditions.checkArgument(taskName == null || !taskName.contains("/"), "'taskName' must not contain '/'");
Review Comment:
@aishikbh - Could you add this to your message
**taskName' must not contain path separator '/'**
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/task/AdhocTaskConfig.java:
##########
@@ -57,6 +57,7 @@ public AdhocTaskConfig(@JsonProperty(value = "taskType", required = true) String
@JsonProperty("taskConfigs") @Nullable Map<String, String> taskConfigs) {
Preconditions.checkArgument(taskType != null, "'taskType' must be configured");
Preconditions.checkArgument(tableName != null, "'tableName' must be configured");
+ Preconditions.checkArgument(taskName == null || !taskName.contains("/"), "'taskName' must not contain '/'");
Review Comment:
Its created outside Helix by PinotTaskManager. We can remove the documentation for custom askName and deprecate it?
if (taskName == null) {
taskName = tableName + "_" + UUID.randomUUID();
LOGGER.info("Task name is missing, auto-generate one: {}", taskName);
}
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
Posted by "aishikbh (via GitHub)" <gi...@apache.org>.
aishikbh commented on code in PR #11955:
URL: https://github.com/apache/pinot/pull/11955#discussion_r1383825809
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/task/AdhocTaskConfig.java:
##########
@@ -57,6 +57,7 @@ public AdhocTaskConfig(@JsonProperty(value = "taskType", required = true) String
@JsonProperty("taskConfigs") @Nullable Map<String, String> taskConfigs) {
Preconditions.checkArgument(taskType != null, "'taskType' must be configured");
Preconditions.checkArgument(tableName != null, "'tableName' must be configured");
+ Preconditions.checkArgument(taskName == null || !taskName.contains("/"), "'taskName' must not contain '/'");
Review Comment:
> @aishikbh - Could you add this to your message **taskName' must not contain path separator '/'**
Sure, will add that!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee commented on code in PR #11955:
URL: https://github.com/apache/pinot/pull/11955#discussion_r1383746812
##########
pinot-spi/src/main/java/org/apache/pinot/spi/config/task/AdhocTaskConfig.java:
##########
@@ -57,6 +57,7 @@ public AdhocTaskConfig(@JsonProperty(value = "taskType", required = true) String
@JsonProperty("taskConfigs") @Nullable Map<String, String> taskConfigs) {
Preconditions.checkArgument(taskType != null, "'taskType' must be configured");
Preconditions.checkArgument(tableName != null, "'tableName' must be configured");
+ Preconditions.checkArgument(taskName == null || !taskName.contains("/"), "'taskName' must not contain '/'");
Review Comment:
Can we double check if the task generator explicitly creates the task name or the helix implicitly generates the task name?
If it's the first case, we should add the validation on the task name generated by the task generator as well. If the name gets created by the helix internally, we wouldn't need to worry about this.
@swaminathanmanish We should consider to remove the task name input from the customer. Ideally, we should internally create the task id.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [bugfix] Add check for illegal character '/' in taskName [pinot]
Posted by "snleee (via GitHub)" <gi...@apache.org>.
snleee merged PR #11955:
URL: https://github.com/apache/pinot/pull/11955
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org