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/08/25 17:25:22 UTC
[GitHub] [druid] abhishekrb19 opened a new pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
abhishekrb19 opened a new pull request #11634:
URL: https://github.com/apache/druid/pull/11634
This change-set does the following:
- Include a new [IPAddress](https://github.com/seancfoley/IPAddress) java library dependency to handle IP addresses.
- Migrate the IPv4 address functions -- `IPV4_MATCH()`, `IPV4_PARSE()`, `IPV4_STRINGIFY()` to use the above package `inet.ipaddr` instead of the `org.apache.commons.net`, `com.google.common.net` and `java.net` packages.
**Rationale:**
- The `org.apache.commons.net.util.SubnetUtils` class doesn't support IPv6 addresses yet. Please see the open JIRA https://issues.apache.org/jira/browse/NET-405. In order to support IPV6 address functions, going down the path of writing custom regexes, parser and longest string matching functions can be tricky given the various IPv6 address formats. The `inet.ipaddr` package supports these right out of the box, so we could leverage that.
- In the future, if there is a desire to expose the IP address parsing options as run time context parameters or similar, it can be plumbed in relatively easily since the java library provides flexibility there.
**Next steps**:
If this looks good, I will go ahead and make an attempt at adding the IPv6 address functions. Open to suggestions. Thanks!
This PR has:
- [ X] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [X ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] 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.
- [ ] added integration tests.
- [X] been tested in a test Druid cluster.
--
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] lgtm-com[bot] commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-906010420
This pull request **introduces 1 alert** and **fixes 2** when merging 70fb9d8f6a61f66cbaed1e8124f2a14f0da9913f into 2a658acad4cd419951d2d9d3f17a67cc26f18727 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-2a6a551ff1ebc39b5cdbf01d9ef21255a028f32b)
**new alerts:**
* 1 for User\-controlled data in numeric cast
**fixed alerts:**
* 2 for User\-controlled data in numeric cast
--
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] clintropolis commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r734872503
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -70,17 +71,17 @@ public Expr apply(final List<Expr> args)
throw new IAE(ExprUtils.createErrMsg(name(), "must have 2 arguments"));
}
- SubnetUtils.SubnetInfo subnetInfo = getSubnetInfo(args);
+ IPAddressString subnetInfo = getSubnetInfo(args);
Expr arg = args.get(0);
class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
{
- private final SubnetUtils.SubnetInfo subnetInfo;
+ private final IPAddressString subnetString;
Review comment:
since this will be checked every time we check a match, should we maybe store both `IPAddressString` and `IPv4Address` so we can do whichever `contains` check is more efficient? (assuming we split long and string handling to not both convert to `IPv4Address`).
--
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] lgtm-com[bot] commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-908749061
This pull request **introduces 1 alert** and **fixes 2** when merging 68acd7748a4c9396ab68d648f99cf760aab7ea78 into a09688862e9257b6651ca35ee0e0515940ba7450 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-734f239fc41615558d77f48899ece8826fff442f)
**new alerts:**
* 1 for User\-controlled data in numeric cast
**fixed alerts:**
* 2 for User\-controlled data in numeric cast
--
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] seancfoley commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
seancfoley commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r746247610
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -104,19 +105,21 @@ public ExprEval eval(final ObjectBinding bindings)
private boolean isStringMatch(String stringValue)
{
- return IPv4AddressExprUtils.isValidAddress(stringValue) && subnetInfo.isInRange(stringValue);
+ IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
Review comment:
If you intend to support IPv6 here at some point in time, you'd want to use IPAddressString here (or possibly IPAddress, super class of IPv4Address and IPv6Address). But since this code here is currently IPv4-specific at this time, you could stick with IPv4Address.
--
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] lgtm-com[bot] commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-905818055
This pull request **introduces 1 alert** and **fixes 2** when merging 85064d2280f1a0e3acc72e100d8a323624756f92 into 2a658acad4cd419951d2d9d3f17a67cc26f18727 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-c8e98dee6f694240133f8a5e7f28c1898a7461c4)
**new alerts:**
* 1 for User\-controlled data in numeric cast
**fixed alerts:**
* 2 for User\-controlled data in numeric cast
--
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] seancfoley commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
seancfoley commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r746247610
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -104,19 +105,21 @@ public ExprEval eval(final ObjectBinding bindings)
private boolean isStringMatch(String stringValue)
{
- return IPv4AddressExprUtils.isValidAddress(stringValue) && subnetInfo.isInRange(stringValue);
+ IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
Review comment:
If you intend to support IPv6 here at some point in time, you'd want to use IPAddressString here. But since it's currently IPv4-specific at this time, you could stick with IPv4Address.
--
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] seancfoley commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
seancfoley commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r746247610
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -104,19 +105,21 @@ public ExprEval eval(final ObjectBinding bindings)
private boolean isStringMatch(String stringValue)
{
- return IPv4AddressExprUtils.isValidAddress(stringValue) && subnetInfo.isInRange(stringValue);
+ IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
Review comment:
If you intend to support IPv6 here at some point in time, you'd want to use IPAddressString here (or possible IPAddress, super class of IPv4Address and IPv6Address). But since this code here is currently IPv4-specific at this time, you could stick with IPv4Address.
--
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] seancfoley commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
seancfoley commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r746247942
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -70,17 +71,17 @@ public Expr apply(final List<Expr> args)
throw new IAE(ExprUtils.createErrMsg(name(), "must have 2 arguments"));
}
- SubnetUtils.SubnetInfo subnetInfo = getSubnetInfo(args);
+ IPAddressString subnetInfo = getSubnetInfo(args);
Expr arg = args.get(0);
class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
{
- private final SubnetUtils.SubnetInfo subnetInfo;
+ private final IPAddressString subnetString;
Review comment:
IPAddressString containment checks are already optimized to be efficient.
--
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] clintropolis commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r734872503
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -70,17 +71,17 @@ public Expr apply(final List<Expr> args)
throw new IAE(ExprUtils.createErrMsg(name(), "must have 2 arguments"));
}
- SubnetUtils.SubnetInfo subnetInfo = getSubnetInfo(args);
+ IPAddressString subnetInfo = getSubnetInfo(args);
Expr arg = args.get(0);
class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
{
- private final SubnetUtils.SubnetInfo subnetInfo;
+ private final IPAddressString subnetString;
Review comment:
since this will be checked every row, should we maybe store both `IPAddressString` and `IPv4Address` so we can do whichever `contains` check is more efficient? (assuming we split long and string handling to not both convert to `IPv4Address`).
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -104,19 +105,21 @@ public ExprEval eval(final ObjectBinding bindings)
private boolean isStringMatch(String stringValue)
{
- return IPv4AddressExprUtils.isValidAddress(stringValue) && subnetInfo.isInRange(stringValue);
+ IPv4Address iPv4Address = IPv4AddressExprUtils.parse(stringValue);
Review comment:
should this parse to an `IPAddressString` instead of an `IPv4Address` so it can be checked against the `subnetString` directly?
--
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] abhishekrb19 commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
abhishekrb19 commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-910736916
@asdf2014, @jihoonson, @clintropolis, this change-set is ready for review. Please take a look whenever you get a chance. Thanks!
--
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] lgtm-com[bot] commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-908749061
This pull request **introduces 1 alert** and **fixes 2** when merging 68acd7748a4c9396ab68d648f99cf760aab7ea78 into a09688862e9257b6651ca35ee0e0515940ba7450 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-734f239fc41615558d77f48899ece8826fff442f)
**new alerts:**
* 1 for User\-controlled data in numeric cast
**fixed alerts:**
* 2 for User\-controlled data in numeric cast
--
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] seancfoley commented on a change in pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
seancfoley commented on a change in pull request #11634:
URL: https://github.com/apache/druid/pull/11634#discussion_r746247942
##########
File path: processing/src/main/java/org/apache/druid/query/expression/IPv4AddressMatchExprMacro.java
##########
@@ -70,17 +71,17 @@ public Expr apply(final List<Expr> args)
throw new IAE(ExprUtils.createErrMsg(name(), "must have 2 arguments"));
}
- SubnetUtils.SubnetInfo subnetInfo = getSubnetInfo(args);
+ IPAddressString subnetInfo = getSubnetInfo(args);
Expr arg = args.get(0);
class IPv4AddressMatchExpr extends ExprMacroTable.BaseScalarUnivariateMacroFunctionExpr
{
- private final SubnetUtils.SubnetInfo subnetInfo;
+ private final IPAddressString subnetString;
Review comment:
IPAddressString containment checks are optimized to be efficient (in some cases more efficient than going straight to IPv4Address). So you could use IPv4Address or IPAddressString, but I'd avoid storing both, there's not much benefit to 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] asdf2014 commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
asdf2014 commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-907988901
Hi @abhishekrb19 , we're on it. Please refer to https://github.com/apache/druid/pull/11638.
--
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] abhishekrb19 commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
abhishekrb19 commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-906583288
hi @jihoonson, @asdf2014, the build is failing in the stage 2 starting with these jobs:
```
32932.42 (Compile=openjdk8, Run=openjdk8) batch index integration test
32932.43 (Compile=openjdk8, Run=openjdk8) batch index integration test with Indexer
...
```
It appears that mysql is not installed/found.
```sh
/bin/sh: 1: /etc/init.d/mysql: not found
```
I see a few jobs as part of other pull requests fail due to the same reason as well. Is this a known issue? Thanks!
--
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] lgtm-com[bot] commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-905927369
This pull request **introduces 1 alert** and **fixes 2** when merging bb576d50138e37998bef9ea73fe7774bfe0a51c2 into 2a658acad4cd419951d2d9d3f17a67cc26f18727 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-9e055451c4e009a4836dace3edf5fc031f1c7629)
**new alerts:**
* 1 for User\-controlled data in numeric cast
**fixed alerts:**
* 2 for User\-controlled data in numeric cast
--
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] lgtm-com[bot] commented on pull request #11634: Add IPAddress java library as dependency and migrate IPv4 functions to use the new library.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #11634:
URL: https://github.com/apache/druid/pull/11634#issuecomment-906208262
This pull request **introduces 1 alert** and **fixes 2** when merging 09cb838e18c1900b374931a0ba36db25e9e38025 into 2a658acad4cd419951d2d9d3f17a67cc26f18727 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-06450328639d07b7fefe72a8f92ff48143b8dcc9)
**new alerts:**
* 1 for User\-controlled data in numeric cast
**fixed alerts:**
* 2 for User\-controlled data in numeric cast
--
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