Looking more carefully at the extent of impact, I am -1 (non-binding) on this change. I understand what you are trying to do, but a) I don't see that the solution actually does it (see comments by Michael in PR) and b) it is going to create a lot of pain for users who have to modify factories, etc. when in fact many / most of them throw unchecked if any exceptions. I would be +1 for removing the throws entirely in 3.0. It does not make sense to me to have a "generic" checked exception type assigned at the pool level. What the different factory and pool methods throw may be different. Even after your change, for example, borrowObject throws NSE. You are changing the signature to make it look like if you catch "E" whatever that is, it will not go bang. But it will if you have replaced "Exception" with a checked exception class. You are just replacing "Exception" with something that looks "cleaner" but is not. Clients will either have to ignore it and just instantiate with "Exception" or create a gratuitous "FooException" to root a hierarchy of their own making (and separately catch the NSEs) when in fact what they throw now are standard exceptions. Remember pool is very widely used so you are asking *many* developers to spend time making changes that many will consider a waste of time. Is the idea that they all just parameterize with "Exception"? Can IDEs do that automatically for them? Please let's get some input from downstream users before surprising them with this.
Phil On Mon, Jun 26, 2023 at 3:55 PM Phil Steitz <phil.ste...@gmail.com> wrote: > > > On Mon, Jun 26, 2023 at 3:43 PM Gary Gregory <garydgreg...@gmail.com> > wrote: > >> Hi Phil, >> >> YW and thank you for the review. >> >> Yes, you are right that this is about POOL-269. While binary compatibility >> is preserved 100%, source compatibility is not. This is one of those rare >> cases where you can't make an omelette without breaking some eggs ;-) >> Since >> binary compatibility is preserved, I don't think this is worth going to a >> new major release. >> > > We should definitely add a *loud* statement in the release notes then and > directions for how to work around because I am sure a lot of downstream > builds will fail due to this as DBCP did. Is there no way that we can > somehow keep the source compatability? Pool is widely used and a lot of > builds will break IIUC what is going on here. > > Phil > >> >> I have POOL projects that will benefit (greatly IMO) from cleaner >> exception >> handling and avoid having to throw/propagate Exception or catch/rethrow >> Exception as a domain Exception, and will now instead be able to use >> domain >> specific exception for resources being pooled from the get go. >> >> I'll obviously adjust DBCP for this as soon as possible (IOW, >> post-release). >> >> For the other items, I will try and reproduce. My tests builds were ok on >> Windows 10 and macOS latest with Java 8. Maybe by hardware is too slow or >> too fast compared to yours, hard to say. >> >> Gary >> >> On Mon, Jun 26, 2023, 16:53 Phil Steitz <phil.ste...@gmail.com> wrote: >> >> > Hi Gary, First, thanks for doing this. There are a lot of good fixes in >> > here. >> > >> > I checked the build, sigs et al on a couple of platforms and did not >> > find anything major except one item. I will start with the >> > show-stopper (IMO) and then the other smaller things. >> > >> > 1. I get compilation failure when I try to compile the latest DBCP >> > release with this code. I think it may have something to do with >> > POOL-269. Here is one example: >> > >> > [*ERROR*] >> > >> /Users/psteitz/Downloads/commons-dbcp2-2.9.0-src/src/main/java/org/apache/commons/dbcp2/PoolableConnectionFactory.java:[45,70] >> > wrong number of type arguments; required 2 >> > >> > This should not happen in a dot release. >> > >> > 2. On MacOS 13.4.1, OpenJDK 20.0.1, I got the following test failure >> > just one time and can't reproduce: >> > >> > java.util.concurrent.ExecutionException: >> > java.lang.NullPointerException: Cannot invoke >> > >> > >> "org.apache.commons.pool2.impl.GenericKeyedObjectPool$ObjectDeque.getIdleObjects()" >> > because the return value of "java.util.Map.get(Object)" is null >> > >> > at >> > java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122) >> > at >> > java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191) >> > at >> > >> org.apache.commons.pool2.impl.TestGenericKeyedObjectPool.lambda$testConcurrentBorrowAndClear$2(TestGenericKeyedObjectPool.java:1056) >> > ... 71 more >> > Caused by: java.lang.NullPointerException: Cannot invoke >> > >> > >> "org.apache.commons.pool2.impl.GenericKeyedObjectPool$ObjectDeque.getIdleObjects()" >> > because the return value of "java.util.Map.get(Object)" is null >> > at >> > >> org.apache.commons.pool2.impl.GenericKeyedObjectPool.addIdleObject(GenericKeyedObjectPool.java:307) >> > at >> > >> org.apache.commons.pool2.impl.GenericKeyedObjectPool.addObject(GenericKeyedObjectPool.java:332) >> > at >> > >> org.apache.commons.pool2.KeyedObjectPool.addObjects(KeyedObjectPool.java:136) >> > at >> > >> org.apache.commons.pool2.KeyedObjectPool.addObjects(KeyedObjectPool.java:113) >> > at >> > >> org.apache.commons.pool2.impl.TestGenericKeyedObjectPool.lambda$testConcurrentBorrowAndClear$0(TestGenericKeyedObjectPool.java:1036) >> > at >> > >> java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:577) >> > at >> > java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317) >> > at >> > >> java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144) >> > at >> > >> java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642) >> > at java.base/java.lang.Thread.run(Thread.java:1623) >> > >> > This one is driving me crazy because the NPE should not be possible >> > due to key registration guarding. I will keep digging on this. >> > >> > 3. >> > >> > I see this one on every run, both config above and Ubuntu 20.0.4 / >> > OpenJDK 11.0.19 >> > >> > Exception in thread "Thread-1305" org.opentest4j.AssertionFailedError >> > >> > at >> > org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:34) >> > >> > at org.junit.jupiter.api.Assertions.fail(Assertions.java:116) >> > >> > at >> > >> org.apache.commons.pool2.impl.TestGenericKeyedObjectPool.lambda$testClearUnblocksWaiters$3(TestGenericKeyedObjectPool.java:1237) >> > >> > at java.base/java.lang.Thread.run(Thread.java:1623) >> > >> > The test that causes it doesn't fail because it happens in a thread >> > that the test spawns and the test does not wait for the threads it >> > starts to finish. Digging into it, I realize this is my sloppiness as >> > I committed this test case. I will make a PR to fix it. I don't >> > think that it indicates a bug. >> > >> > 4. I forgot to add since tag on the new version of GKOP clear that >> > adds reuseCapacity parm. Will fix in the same PR. >> > >> > 5. Small nit. There is a vestigal paragraph in the release notes >> > template that I think should be dropped: >> > >> > >> > No client code changes are required to migrate from versions 2.0-2.3 >> > to version 2.4.3. >> > >> > Users of version 1.x should consult the migration guide on the Commons >> > Pool web site. >> > >> > Phil >> > >> > On Sat, Jun 24, 2023 at 6:27 PM Gary Gregory <garydgreg...@gmail.com> >> > wrote: >> > >> > > We have fixed quite a few bugs and added some enhancements since >> > > Apache Commons Pool 2.11.1 was released, so I would like to release >> > > Apache Commons Pool 2.12.0. >> > > >> > > Apache Commons Pool 2.12.0 RC1 is available for review here: >> > > https://dist.apache.org/repos/dist/dev/commons/pool/2.12.0-RC1 >> > > (svn revision 62626) >> > > >> > > The Git tag commons-pool-2.12.0-RC1 commit for this RC is >> > > e5dae53e0ce1211b40680e7dccf601c3c3897378 which you can browse here: >> > > >> > > >> > >> https://gitbox.apache.org/repos/asf?p=commons-pool.git;a=commit;h=e5dae53e0ce1211b40680e7dccf601c3c3897378 >> > > You may checkout this tag using: >> > > git clone https://gitbox.apache.org/repos/asf/commons-pool.git >> > > --branch < >> https://gitbox.apache.org/repos/asf/commons-pool.git--branch> >> > > commons-pool-2.12.0-RC1 commons-pool-2.12.0-RC1 >> > > >> > > Maven artifacts are here: >> > > >> > > >> > >> https://repository.apache.org/content/repositories/orgapachecommons-1640/org/apache/commons/commons-pool2/2.12.0/ >> > > >> > > These are the artifacts and their hashes: >> > > >> > > #Release SHA-512s >> > > #Sat Jun 24 21:12:18 EDT 2023 >> > > >> > > >> > >> commons-pool2-2.12.0-bin.tar.gz=e16b3a81c98feae6f9855a9bd2f2226dae51558c6e7bb77ee626e58853420ccc59d0943a594bba27ab7147524eca823cac47484e304ebaf14bd724b96bbffc25 >> > > >> > > >> > >> commons-pool2-2.12.0-bin.zip=29339f89a8efaa4ad3efbe656d610b8951be1a2a005ba7cb58ae356660331af9d473e8ada8bcdedd3d765c54d1dcfee8779ccb3902f0220de7e92e3039f95b8d >> > > >> > > >> > >> commons-pool2-2.12.0-bom.json=adb3e197d360dc7f53ab116c8fa8b1699d60560fcf977cae613c4cb493168a130ca8041d4ca475e75386f281688819fa5f5111e4aa937b24390a6de8e779f507 >> > > >> > > >> > >> commons-pool2-2.12.0-bom.xml=d964e9ec5ed51c10591093bdd0e174f8eb354a447f710e8c3749100fdbda7456e3922846a7190180e5044fe46e571cbc600aaea1b8f64a37c12c5deaa2f1662a >> > > >> > > >> > >> commons-pool2-2.12.0-javadoc.jar=356891b25f2e0367b74a7c4070c26d3cec7a3e608b6e47205e5ffeface590c9717187cb1fa72ebb4c484adaac2c7634ff1944c88282ba9c6551ab5abb58c87f4 >> > > >> > > >> > >> commons-pool2-2.12.0-sources.jar=6d955b437496d7af6f94844010a1df15efc04e2b9c15fadc1001777c54a60433570744605b0625a21adf53f03ce9e339809977384c562ad357a98370749c8ee6 >> > > >> > > >> > >> commons-pool2-2.12.0-src.tar.gz=eed0575d8357349c908fe8539db2c8ef23234c306f373d203d3d2d9a4ee1ae51cb6bdb2f86163e2296ac90f67a27c7d8cfde239cdfc8ab4966c6239b63f5984e >> > > >> > > >> > >> commons-pool2-2.12.0-src.zip=d8158fd14ee393a99dd0abcb55448b699182a50e0ea114cd3a2681799c9f5e58161f71a2420d0605d1ea39efb08c31be1f7a38531c169fd2c69cc604458ee184 >> > > >> > > >> > >> commons-pool2-2.12.0-test-sources.jar=db4fabab1fae77e5dcf8af35397635738c6296ebc25065be9a73725d6b837179c3973ae3ea531aa40056459f011ff90e3c2ef16ad2fa77114dacaa5709e3bf57 >> > > >> > > >> > >> commons-pool2-2.12.0-tests.jar=81af180ba6d2a5ce12064c9cc4eda4bc25d072fef55a3dc7ce48506571d40aceb6636c80202c31eff867eb8fd1971a44e0c2c2979019a1e32f43005d70cf2f5e >> > > >> > > >> > >> org.apache.commons_commons-pool2-2.12.0.spdx.json=ed49a8ca7a776ede454f8765f1bd71b5d6a2da35b8bd46ebe930f663127fbd6f248c27059b417ca578665295113ca56a0f0e6486c92436b7b9af7984e3f111db >> > > >> > > I have tested this with: >> > > >> > > mvn -V -Duser.name=$my_apache_id >> > > -Dcommons.release-plugin.version=$commons_release_plugin_version >> > > -Prelease -Ptest-deploy -P jacoco -P japicmp clean package site deploy >> > > >> > > using: >> > > >> > > Apache Maven 3.9.2 (c9616018c7a021c1c39be70fb2843d6f5f9b8a1c) >> > > Maven home: /usr/local/Cellar/maven/3.9.2/libexec >> > > Java version: 1.8.0_372, vendor: Homebrew, runtime: >> > > /usr/local/Cellar/openjdk@8 >> > > /1.8.0+372/libexec/openjdk.jdk/Contents/Home/jre >> > > Default locale: en_US, platform encoding: UTF-8 >> > > OS name: "mac os x", version: "13.4.1", arch: "x86_64", family: "mac" >> > > Darwin gdg-mac-mini.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun >> > > 8 22:22:22 PDT 2023; root:xnu-8796.121.3~7/RELEASE_X86_64 x86_64 >> > > >> > > Details of changes since 2.11.1 are in the release notes: >> > > >> > > >> > >> https://dist.apache.org/repos/dist/dev/commons/pool/2.12.0-RC1/RELEASE-NOTES.txt >> > > >> > > >> > >> https://dist.apache.org/repos/dist/dev/commons/pool/2.12.0-RC1/site/changes-report.html >> > > >> > > Site: >> > > >> > > >> > >> https://dist.apache.org/repos/dist/dev/commons/pool/2.12.0-RC1/site/index.html >> > > (note some *relative* links are broken and the 2.12.0 directories >> > > are not yet created - these will be OK once the site is deployed.) >> > > >> > > JApiCmp Report (compared to 2.11.1): >> > > >> > > >> > >> https://dist.apache.org/repos/dist/dev/commons/pool/2.12.0-RC1/site/japicmp.html >> > > >> > > RAT Report: >> > > >> > > >> > >> https://dist.apache.org/repos/dist/dev/commons/pool/2.12.0-RC1/site/rat-report.html >> > > >> > > KEYS: >> > > https://www.apache.org/dist/commons/KEYS >> > > >> > > Please review the release candidate and vote. >> > > This vote will close no sooner than 72 hours from now. >> > > >> > > [ ] +1 Release these artifacts >> > > [ ] +0 OK, but... >> > > [ ] -0 OK, but really should fix... >> > > [ ] -1 I oppose this release because... >> > > >> > > Thank you, >> > > >> > > Gary Gregory, >> > > Release Manager (using key 86fdc7e2a11262cb) >> > > >> > > For following is intended as a helper and refresher for reviewers. >> > > >> > > Validating a release candidate >> > > ============================== >> > > >> > > These guidelines are NOT complete. >> > > >> > > Requirements: Git, Java, Maven. >> > > >> > > You can validate a release from a release candidate (RC) tag as >> follows. >> > > >> > > 1) Clone and checkout the RC tag >> > > >> > > git clone https://gitbox.apache.org/repos/asf/commons-pool.git >> > > --branch commons-pool-2.12.0-RC1 commons-pool-2.12.0-RC1 >> > > cd commons-pool-2.12.0-RC1 >> > > >> > > 2) Check Apache licenses >> > > >> > > This step is not required if the site includes a RAT report page which >> > > you then must check. >> > > >> > > mvn apache-rat:check >> > > >> > > 3) Check binary compatibility >> > > >> > > Older components still use Apache Clirr: >> > > >> > > This step is not required if the site includes a Clirr report page >> > > which you then must check. >> > > >> > > mvn clirr:check >> > > >> > > Newer components use JApiCmp with the japicmp Maven Profile: >> > > >> > > This step is not required if the site includes a JApiCmp report page >> > > which you then must check. >> > > >> > > mvn install -DskipTests -P japicmp japicmp:cmp >> > > >> > > 4) Build the package >> > > >> > > mvn -V clean package >> > > >> > > You can record the Maven and Java version produced by -V in your VOTE >> > > reply. >> > > To gather OS information from a command line: >> > > Windows: ver >> > > Linux: uname -a >> > > >> > > 5) Build the site for a single module project >> > > >> > > Note: Some plugins require the components to be installed instead of >> > > packaged. >> > > >> > > mvn site >> > > Check the site reports in: >> > > - Windows: target\site\index.html >> > > - Linux: target/site/index.html >> > > >> > > -the end- >> > > >> > > --------------------------------------------------------------------- >> > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> > > For additional commands, e-mail: dev-h...@commons.apache.org >> > > >> > > >> > >> >
