zacharymorn commented on pull request #2052: URL: https://github.com/apache/lucene-solr/pull/2052#issuecomment-748506650
> Hi, I implemented the approach and also cleaned up the lookup code to not require unchecked casts. I also made the code safer for exceptions, you only catched ClassNotFound, but actually there can more go wrong! > > * Cast the class after forName() to OpenOption. Then the stream knows what everything is and we can bail out soon. > * catch all Exceptions, as there can happy many: ClassNotFound, ClassCast (if it's not an OpenOption), NullPointerException (if it's not an enum). In static initializers we should not risk any of this. It's just not supported if ANYTHING goes wrong. - BASTA 👍 > > I also added an assume to the test, so it won't run without that open option. > > I also changed the Exception to UnsupportedOperationException, as this is what the Javadocs say in the class description. This is also in line with "operating system does not support it" behaviour of JDK, which throws UOE in the open method. > > There's one TODO: If we change to Java module system, we may need to add an import to the unsupported module. Wow thanks @uschindler ! The changes look great! I think there are some java doc I put in earlier that use IOException instead of UOE, I can push a commit fast to update them as well. > Nevertheless, on Windows I see the test failing all the time (JDK 11 and JDK 15 were tested): > > ``` > org.apache.lucene.misc.store.TestDirectIODirectory > testWriteReadWithDirectIO FAILED > java.nio.BufferOverflowException > at __randomizedtesting.SeedInfo.seed([667380F7FFC63E9E:3A56F8A55C75455F]:0) > at java.base/java.nio.DirectByteBuffer.put(DirectByteBuffer.java:409) > at org.apache.lucene.misc.store.DirectIODirectory$DirectIOIndexOutput.writeBytes(DirectIODirectory.java:210) > at org.apache.lucene.misc.store.TestDirectIODirectory.testWriteReadWithDirectIO(TestDirectIODirectory.java:43) > ``` > > The other implicit tests seem to work. Maybe this one stresses the block sizes too much. Hmm I don't have a windows box to test this. The `blockSize` is dynamically computed though. If this works on linux/darwin but fails on windows, does it signal another bug in the JDK implementation (since there's butter overflow?), or just some platform specific condition we also need to handle? > The master branch of the JDK always emits this exception, it seems: > https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java#L3534-L3540 > > Previously it was guarded with 'sunapi' lint switch. To be honest this looks like a regression because the sunApiHandler is still there in the code, it's just never used: > https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java#L149-L158 Thanks @dweiss for looking it up! It seems that change was introduced here more than 4 years ago https://github.com/openjdk/jdk/commit/286b0caa6cf0ac85128004c091d8d508421389f1 . However, this doesn't seems to explain what were observed in the testings though, where javac and --release using different version causing the warning to be emitted, even when no -Xlint option is provided (the new code looks like the warning message should never be emitted, like in the case where the two versions match, but only logged?). ---------------------------------------------------------------- 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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
