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/02/25 18:57:14 UTC
[GitHub] [zookeeper] ctubbsii opened a new pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
ctubbsii opened a new pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269
Remove unsupported use of com.sun.nio.file.SensitivityWatchEventModifier
to better support builds against newer JDKs.
Also update build tooling to use strict JDK release compatibility when
building with newer JDKs by adding profiles which automatically activate
the correct compiler flag when the newer JDK is detected when building
with Maven or Eclipse.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] nkalmar commented on issue #1269: ZOOKEEPER-3739:
Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
nkalmar commented on issue #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#issuecomment-601618510
merged to master and branch-3.6, thanks @ctubbsii
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r384163550
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
> Can't we detect the presence of that option at runtime and use it with reflection?
Code to do that would look similar to what is described in:
https://github.com/HotswapProjects/HotswapAgent/issues/41#issue-39256602
However, because of module access restrictions in future JVMs, there's a strong possibility that doing this would result in runtime exceptions in newer JVMs, unless users added the `jdk.unsupported` module to their module path at runtime... and even then, these unsupported APIs are supposed to be phased out entirely at some point... so there's a good chance this will add complexity which will never matter, because the complex code path will never be traversed (or will be traversed and cause an access restriction error at runtime).
> What's the impact of this patch?
That's the question I had, too (and the related: is it even worth it to try to keep the modifier?).
From what I could tell, the behavior this modifies is not well defined anyway. On some systems, the modifier *could* make file changes observed more quickly... but there's no guarantee of that... and there's no guarantee of seeing *every* change event on the file being watched, no matter what modifier is on it. Even with the modifier, it's probably still going to be on the order of a few seconds to notice changes. In my opinion, it's probably not worth it, but I'm not an expert on NIO stuff by far.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] asfgit closed pull request #1269: ZOOKEEPER-3739:
Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r389372208
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
@eolivelli If @lvfangmin and @enixon do not reply, what happens next?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r386560053
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
Looking into this a little bit more, it seems this utility is only ever used by the X509Util class to monitor changes to keyStore and trustStore files on disk, presumably for some sort of automatic reconfiguration.
The modifier seems to be an attempt to work around the slow polling implementation of the watch service on Mac OS X (https://bugs.openjdk.java.net/browse/JDK-7133447), which doesn't have a native implementation on that platform.
I can't imagine those files changing frequently enough for this to matter. But, even if they did change, I can't imagine that the optimization to automatically reconfigure a few seconds sooner matters either.
However, if rapid automatic TLS reconfiguration on OSX was a critical need for somebody... there are lots of alternative watch service implementations out there to work around this issue. One is https://github.com/takari/directory-watcher ; I'm not advocating for using any of these... I don't know enough about them, but if it was important for somebody, they could consider contributing a change that makes use of one of those, instead of relying on the com.sun modifier to the built-in watch service that this PR removes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on issue #1269: ZOOKEEPER-3739:
Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#issuecomment-596224742
> We have to wait for any other committer to approve this patch.
> If you want you can ping for review on [dev@zookeeper.apache.org](mailto:dev@zookeeper.apache.org)
Okay. I [tried that a few days ago](https://lists.apache.org/thread.html/r90d2fbdd3551af8d9844d76a64adc77da250b2e54c4e99afeec188bf%40%3Cdev.zookeeper.apache.org%3E) and didn't get a response. Sorry for the impatience :smiley_cat:
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r386560053
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
Looking into this a little bit more, it seems this utility is only ever used by the X509Util class to monitor changes to keyStore and trustStore files on disk, presumably for some sort of automatic reconfiguration.
The modifier seems to be an attempt to work around the slow polling implementation of the watch service on Mac OS X (https://bugs.openjdk.java.net/browse/JDK-7133447), which doesn't have a native implementation on that platform.
I can't imagine those files changing frequently enough for this to matter. But, even if they did change, I can't imagine that the optimization to automatically reconfigure a few seconds sooner matters either.
However, if automatic TLS reconfiguration was a critical need for somebody... there are lots of alternative watch service implementations out there to work around this issue. One is https://github.com/takari/directory-watcher ; I'm not advocating for using any of these... I don't know enough about them, but if it was important for somebody, they could consider contributing a change that makes use of one of those, instead of relying on the com.sun modifier to the built-in watch service that this PR removes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on issue #1269: ZOOKEEPER-3739:
Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
eolivelli commented on issue #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#issuecomment-596220474
We have to wait for any other committer to approve this patch.
If you want you can ping for review on dev@zookeeper.apache.org
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r384414628
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
I think this code was contributed from Facebook
@lvfangmin @enixon do you have any insight ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r384125400
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
Can't we detect the presence of that option at runtime and use it with reflection?
What's the impact of this patch?
Cc @enixon PTAL
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r395533463
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/FileChangeWatcher.java
##########
@@ -72,7 +71,7 @@ public FileChangeWatcher(Path dirPath, Consumer<WatchEvent<?>> callback) throws
LOG.debug("Registering with watch service: {}", dirPath);
- dirPath.register(watchService, new WatchEvent.Kind<?>[]{StandardWatchEventKinds.ENTRY_CREATE, StandardWatchEventKinds.ENTRY_DELETE, StandardWatchEventKinds.ENTRY_MODIFY, StandardWatchEventKinds.OVERFLOW}, SensitivityWatchEventModifier.HIGH);
Review comment:
Digging into this I also found a bunch of patch for OSX to speed up file change event.
There is not much info if this is still useful at all. In fact, most source (patch, stackoverflow etc.) I found are a few years old.
And in any case, I don't think there are many running production ZK on OSX. (If this change would primarily affect Linux systems I would be more keen on doing further research)
In short, LGTM
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1269:
ZOOKEEPER-3739: Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#discussion_r384125516
##########
File path: pom.xml
##########
@@ -307,6 +307,26 @@
</plugins>
</build>
</profile>
+ <profile>
+ <id>m2e</id>
+ <activation>
+ <property>
+ <name>m2e.version</name>
+ </property>
+ </activation>
+ <properties>
+ <maven.compiler.release>8</maven.compiler.release>
+ </properties>
+ </profile>
+ <profile>
+ <id>jdk-release-flag</id>
+ <activation>
+ <jdk>[9,)</jdk>
+ </activation>
+ <properties>
+ <maven.compiler.release>8</maven.compiler.release>
Review comment:
Good
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [zookeeper] ctubbsii commented on issue #1269: ZOOKEEPER-3739:
Remove unsupported com.sun API
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on issue #1269: ZOOKEEPER-3739: Remove unsupported com.sun API
URL: https://github.com/apache/zookeeper/pull/1269#issuecomment-601774833
Cool, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services