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 2022/03/08 00:30:37 UTC
[GitHub] [druid] churromorales opened a new pull request #12316: Adding k8s support for human readable parsing
churromorales opened a new pull request #12316:
URL: https://github.com/apache/druid/pull/12316
currently druid only supports decimal format 'k', 'm', 'g', etc...
or
binary byte format 'kib', 'mib', 'gib', etc....
When dealing with kubernetes they use the format with the decimal format + `i` only no `b` suffix for binary format.
We encountered this problem when having a volume of some size configured in helm like `500Gi`
Then wanted to programatically assign the `druid.segmentCache.locations` by taking the volumeclaimtemplate's size and using it for the `maxSize` parameter. This failed because in k8s you don't refer to it as `500GiB` instead its just `500Gi`
Would be nice to have druid be able to parse input in this format 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@druid.apache.org
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] FrankChen021 commented on pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #12316:
URL: https://github.com/apache/druid/pull/12316#issuecomment-1067481485
Just push new commits, no need to squash them into one, and no forced push. All commits will be squashed into one during merging.
--
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@druid.apache.org
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] FrankChen021 commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r824811271
##########
File path: docs/configuration/human-readable-byte.md
##########
@@ -64,13 +64,18 @@ To make it clear, the base of units are defined in Druid as below
| G | Giga Decimal Byte | 1_000_000_000 |
| T | Tera Decimal Byte | 1_000_000_000_000 |
| P | Peta Decimal Byte | 1_000_000_000_000_000 |
+| Ki | Kilo Binary Byte | 1024 |
+| Mi | Mega Binary Byte | 1024 * 1024 |
+| Gi | Giga Binary Byte | 1024 * 1024 * 1024 |
+| Ti | Tera Binary Byte | 1024 * 1024 * 1024 * 1024 |
+| Pi | Peta Binary Byte | 1024 * 1024 * 1024 * 1024 * 1024 |
| KiB | Kilo Binary Byte | 1024 |
| MiB | Mega Binary Byte | 1024 * 1024 |
| GiB | Giga Binary Byte | 1024 * 1024 * 1024 |
| TiB | Tera Binary Byte | 1024 * 1024 * 1024 * 1024 |
| PiB | Peta Binary Byte | 1024 * 1024 * 1024 * 1024 * 1024 |
-Unit is case-insensitive. `k`, `kib`, `KiB`, `kiB` are all acceptable.
+Unit is case-insensitive. `k`, `kib`, `ki`, `KiB`, `Ki`, `kiB` are all acceptable.
Here are two examples
Review comment:
```suggestion
Here are some examples
```
--
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@druid.apache.org
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] FrankChen021 commented on pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #12316:
URL: https://github.com/apache/druid/pull/12316#issuecomment-1065211662
Thank you for your first contribution. I left some comments. Feel free to let me know if you have any questions.
--
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@druid.apache.org
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] FrankChen021 commented on pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #12316:
URL: https://github.com/apache/druid/pull/12316#issuecomment-1061347337
I will take a look later this week. Thank you.
--
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@druid.apache.org
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] FrankChen021 commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r824811792
##########
File path: docs/configuration/human-readable-byte.md
##########
@@ -84,5 +89,10 @@ druid.cache.sizeInBytes=1g
druid.cache.sizeInBytes=256MiB
```
+```properties
+# 256MiB bytes = 256 * 1024 * 1024 bytes
Review comment:
```suggestion
# 256Mi = 256MiB = 256 * 1024 * 1024 bytes
```
--
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@druid.apache.org
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] FrankChen021 commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r824794808
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -159,10 +159,18 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format of number: %s", rawNumber);
}
+ unit = number.charAt(lastDigitIndex--);
+ isBinaryByte = true;
+ } else if (unit == 'i') {
+ //unit ends with 'i' must be format of Ki/Mi/Gi/Ti/Pi, so at least 2 extra characters are required
+ if (lastDigitIndex < 1) {
+ throw new IAE("Invalid format of number: %s", rawNumber);
+ }
unit = number.charAt(lastDigitIndex--);
isBinaryByte = true;
}
+
Review comment:
accidental change?
--
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@druid.apache.org
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] FrankChen021 commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r824815122
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -159,10 +159,18 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format of number: %s", rawNumber);
}
+ unit = number.charAt(lastDigitIndex--);
+ isBinaryByte = true;
+ } else if (unit == 'i') {
+ //unit ends with 'i' must be format of Ki/Mi/Gi/Ti/Pi, so at least 2 extra characters are required
+ if (lastDigitIndex < 1) {
+ throw new IAE("Invalid format of number: %s", rawNumber);
Review comment:
Exception messages above are not instructive, so I think it's better to improve it as something like
```suggestion
throw new IAE("Invalid format of number [%s]. The unit should be one of Pi/Ti/Gi/Mi/Ki", rawNumber);
```
Could you also improve the messages at Line 156 and 159?
--
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@druid.apache.org
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] FrankChen021 commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r824815122
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -159,10 +159,18 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format of number: %s", rawNumber);
}
+ unit = number.charAt(lastDigitIndex--);
+ isBinaryByte = true;
+ } else if (unit == 'i') {
+ //unit ends with 'i' must be format of Ki/Mi/Gi/Ti/Pi, so at least 2 extra characters are required
+ if (lastDigitIndex < 1) {
+ throw new IAE("Invalid format of number: %s", rawNumber);
Review comment:
Exception messages above are not instructive, so I think it's better to improve it as something like
```suggestion
throw new IAE("Invalid format of number [%s]. The unit should be one of Pi/Ti/Gi/Mi/Ki", rawNumber);
```
--
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@druid.apache.org
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] FrankChen021 commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r824810724
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -227,7 +235,12 @@ private static long parseInner(String rawNumber)
* simplified SI format without 'B' indicator
* eg: K, M, G ...
*/
- DECIMAL
+ DECIMAL,
+ /**
+ * same as binary byte format, that k8s uses
+ * eg: Ki, Mi, Gi ...
+ */
+ KUBERNETES_BYTE
Review comment:
These enums are designed for the format function `HUMAN_READABLE_XXXX_FORMAT` on SQL. At the SQL layer, since you don't add a new SQL function, I think we don't need to add format support here. Another reason is that I think at the SQL layer, it's better keeping the standard format function only.
--
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@druid.apache.org
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] FrankChen021 merged pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 merged pull request #12316:
URL: https://github.com/apache/druid/pull/12316
--
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@druid.apache.org
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] churromorales commented on pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
churromorales commented on pull request #12316:
URL: https://github.com/apache/druid/pull/12316#issuecomment-1067062168
thank you for taking the time to review, all the comments make sense to me and ill make the appropriate changes and push up new commit(s). Is that how you guys like it? Or squashed into a single?
--
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@druid.apache.org
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] churromorales commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
churromorales commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r826158661
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -159,10 +159,18 @@ private static long parseInner(String rawNumber)
throw new IAE("Invalid format of number: %s", rawNumber);
}
+ unit = number.charAt(lastDigitIndex--);
+ isBinaryByte = true;
+ } else if (unit == 'i') {
+ //unit ends with 'i' must be format of Ki/Mi/Gi/Ti/Pi, so at least 2 extra characters are required
+ if (lastDigitIndex < 1) {
+ throw new IAE("Invalid format of number: %s", rawNumber);
+ }
unit = number.charAt(lastDigitIndex--);
isBinaryByte = true;
}
+
Review comment:
yes, i will fix 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@druid.apache.org
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] churromorales commented on a change in pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
churromorales commented on a change in pull request #12316:
URL: https://github.com/apache/druid/pull/12316#discussion_r826159532
##########
File path: core/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java
##########
@@ -227,7 +235,12 @@ private static long parseInner(String rawNumber)
* simplified SI format without 'B' indicator
* eg: K, M, G ...
*/
- DECIMAL
+ DECIMAL,
+ /**
+ * same as binary byte format, that k8s uses
+ * eg: Ki, Mi, Gi ...
+ */
+ KUBERNETES_BYTE
Review comment:
yeah sure, ill remove this and corresponding tests. I didn't know if it was used anywhere else from a config standpoint to do any conversion. But if it is only for SQL better to remove it.
--
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@druid.apache.org
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] FrankChen021 closed pull request #12316: Adding k8s support for human readable parsing
Posted by GitBox <gi...@apache.org>.
FrankChen021 closed pull request #12316:
URL: https://github.com/apache/druid/pull/12316
--
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@druid.apache.org
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