You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/03 22:52:08 UTC
[GitHub] [zookeeper] tamaashu opened a new pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
tamaashu opened a new pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478
Removed commons-lang from main project, replaced functionality with standard Java code.
Updated commons-lang in ZooInspector to latest (to ise it instead of outdated transitive dependency).
Change-Id: Id8586949a897d1d99c7f72f00b68d212e7f51064
----------------------------------------------------------------
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
[GitHub] [zookeeper] TisonKun commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499688257
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private static String[] split(final String path){
+ return Stream.of(path.split("/"))
+ .filter(t -> (t != null && !t.trim().isEmpty()))
Review comment:
Fine.
----------------------------------------------------------------
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
[GitHub] [zookeeper] asfgit closed pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#issuecomment-704181783
OK, I'll merge then to master and try (build / test) the change on branch-3.6 too.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499376188
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private static String[] split(final String path){
+ return Stream.of(path.split("/"))
+ .filter(t -> (t != null && !t.trim().isEmpty()))
Review comment:
To get equivalent behavior, it looks like you could probably just do:
```suggestion
.filter(t -> !t.isEmpty())
```
This is because `StringUtils.split` can't return a `null` element, and it removes truly empty elements (redundant delimiters). I don't think it normally does a trim on each element, though.
To me, it looks like the proposed changes include reasonable safeguards that are worth keeping. Do you really want elements that include only whitespace?
----------------------------------------------------------------
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
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499450579
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private static String[] split(final String path){
+ return Stream.of(path.split("/"))
+ .filter(t -> (t != null && !t.trim().isEmpty()))
Review comment:
> is this filter semantically different from commons-lang's `StringUtils.split`? IIRC validation has been done before passed into `PathTrie` or at least this method seems more than just split path but also validate and filter things.
If we split a path like "/node1/node2" with StringUtils.split at "/" we get the following: ["node1", "node2"].
If we do the same with String.split we get ["", "node1", "node2"], because it recognizes an empty string before the first "/".
The filter provides a solution to not have the first empty string, only the rest of the elements.
----------------------------------------------------------------
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
[GitHub] [zookeeper] tamaashu commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
tamaashu commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499452923
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private static String[] split(final String path){
+ return Stream.of(path.split("/"))
+ .filter(t -> (t != null && !t.trim().isEmpty()))
Review comment:
> To get equivalent behavior, it looks like you could probably just do:
>
> This is because `StringUtils.split` can't return a `null` element, and it removes truly empty elements (redundant delimiters). I don't think it normally does a trim on each element, though.
>
> To me, it looks like the proposed changes include reasonable safeguards that are worth keeping. Do you really want elements that include only whitespace?
Good catch, thanks. Updated.
----------------------------------------------------------------
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
[GitHub] [zookeeper] TisonKun commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499367641
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private static String[] split(final String path){
+ return Stream.of(path.split("/"))
+ .filter(t -> (t != null && !t.trim().isEmpty()))
Review comment:
is this filter semantically different from commons-lang's `StringUtils.split`? IIRC validation has been done before passed into `PathTrie` or at least this method seems more than just split path but also validate and filter things.
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499218685
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private String[] split(final String path){
Review comment:
Nit: static
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#issuecomment-704195415
I merged it to branch-3.6 too (the cherry-pick was clean, also verified with: `mvn verify spotbugs:check checkstyle:check -Pfull-build -Dsurefire-forkcount=4`)
----------------------------------------------------------------
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
[GitHub] [zookeeper] symat commented on pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#issuecomment-704162331
which branch should we push this? I'm OK to push this to branch-3.6 too, as it is a low risk change and potentially help us to avoid CVEs. @eolivelli what do you think?
----------------------------------------------------------------
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
[GitHub] [zookeeper] eolivelli commented on pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#issuecomment-704165670
I am okay with branch-3.6.
----------------------------------------------------------------
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
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1478: ZOOKEEPER-3952: Remove commons-lang from ZooKeeper
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1478:
URL: https://github.com/apache/zookeeper/pull/1478#discussion_r499376188
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/PathTrie.java
##########
@@ -344,4 +345,10 @@ public void clear() {
}
}
+ private static String[] split(final String path){
+ return Stream.of(path.split("/"))
+ .filter(t -> (t != null && !t.trim().isEmpty()))
Review comment:
To get equivalent behavior, it looks like you could probably just do:
```suggestion
.filter(t -> !t.isEmpty())
```
But, to me, it looks like the proposed changes include reasonable safeguards that are worth keeping.
----------------------------------------------------------------
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