Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154
Hello, I do agree that we don’t need to worry about removing synchronized for the purpose of beeing compatible with early versions of Loom (at least not for all commons projects). This is especially true if the code gets more ugly, might have subtle behavior changes or similar. However I think in the context of the PR it looks like the existing code did not use synchronize, so it would be good to not change it to do so (especially not if that’s not needed for the change in question!). I did not follow the changes completely, so I am not sure what’s proposed. Can we we maybe squash it at minimize the changes to fix the actual Bug (if there is one, since I think we still have no specification on concurrency and locking properties of VFS) and keep them Loom support discussion separate from the release? Gruss Bernd -- http://bernd.eckenfels.net Von: Gary Gregory Gesendet: Tuesday, March 2, 2021 7:31:01 PM An: Commons Developers List Betreff: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154 I want to move the discussion from the PR to this mailing list, https://github.com/apache/commons-vfs/pull/154 TY, Gary
Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154
I plan on cutting a release candidate this week but I do not see this PR as a blocker, just a nice-to-have if appropriate. I encourage all to review PRs and Jiras. I like RERO so if you guys can only help later, that's fine as well. Gary On Tue, Mar 2, 2021, 13:46 Bernd Eckenfels wrote: > Hello, > > I do agree that we don’t need to worry about removing synchronized for the > purpose of beeing compatible with early versions of Loom (at least not for > all commons projects). This is especially true if the code gets more ugly, > might have subtle behavior changes or similar. > > However I think in the context of the PR it looks like the existing code > did not use synchronize, so it would be good to not change it to do so > (especially not if that’s not needed for the change in question!). > > I did not follow the changes completely, so I am not sure what’s proposed. > Can we we maybe squash it at minimize the changes to fix the actual Bug (if > there is one, since I think we still have no specification on concurrency > and locking properties of VFS) and keep them Loom support discussion > separate from the release? > > Gruss > Bernd > -- > http://bernd.eckenfels.net > > Von: Gary Gregory > Gesendet: Tuesday, March 2, 2021 7:31:01 PM > An: Commons Developers List > Betreff: [VFS] Consensus needed for > https://github.com/apache/commons-vfs/pull/154 > > I want to move the discussion from the PR to this mailing list, > https://github.com/apache/commons-vfs/pull/154 > > TY, > Gary >
Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154
Note that the PR fixes issues found by Spotbugs/PMD. The current in master sync on the lock obj looks like an oddity... and it's confusing especially if intentional as it's not commented to "double up" the use of the lock object as both a real lock and the use of its monitor. I'm tending to merge it unless someone proposes an alternative that also addresses Spotbugs/PMD issues. Gary On Tue, Mar 2, 2021, 14:10 Gary Gregory wrote: > I plan on cutting a release candidate this week but I do not see this PR > as a blocker, just a nice-to-have if appropriate. > > I encourage all to review PRs and Jiras. > > I like RERO so if you guys can only help later, that's fine as well. > > Gary > > > On Tue, Mar 2, 2021, 13:46 Bernd Eckenfels wrote: > >> Hello, >> >> I do agree that we don’t need to worry about removing synchronized for >> the purpose of beeing compatible with early versions of Loom (at least not >> for all commons projects). This is especially true if the code gets more >> ugly, might have subtle behavior changes or similar. >> >> However I think in the context of the PR it looks like the existing code >> did not use synchronize, so it would be good to not change it to do so >> (especially not if that’s not needed for the change in question!). >> >> I did not follow the changes completely, so I am not sure what’s >> proposed. Can we we maybe squash it at minimize the changes to fix the >> actual Bug (if there is one, since I think we still have no specification >> on concurrency and locking properties of VFS) and keep them Loom support >> discussion separate from the release? >> >> Gruss >> Bernd >> -- >> http://bernd.eckenfels.net >> >> Von: Gary Gregory >> Gesendet: Tuesday, March 2, 2021 7:31:01 PM >> An: Commons Developers List >> Betreff: [VFS] Consensus needed for >> https://github.com/apache/commons-vfs/pull/154 >> >> I want to move the discussion from the PR to this mailing list, >> https://github.com/apache/commons-vfs/pull/154 >> >> TY, >> Gary >> >
Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154
Closing the loop on this thread as this PR has been merged. Gary On Wed, Mar 3, 2021, 13:06 Gary Gregory wrote: > Note that the PR fixes issues found by Spotbugs/PMD. The current in master > sync on the lock obj looks like an oddity... and it's confusing especially > if intentional as it's not commented to "double up" the use of the lock > object as both a real lock and the use of its monitor. > > I'm tending to merge it unless someone proposes an alternative that also > addresses Spotbugs/PMD issues. > > Gary > > On Tue, Mar 2, 2021, 14:10 Gary Gregory wrote: > >> I plan on cutting a release candidate this week but I do not see this PR >> as a blocker, just a nice-to-have if appropriate. >> >> I encourage all to review PRs and Jiras. >> >> I like RERO so if you guys can only help later, that's fine as well. >> >> Gary >> >> >> On Tue, Mar 2, 2021, 13:46 Bernd Eckenfels >> wrote: >> >>> Hello, >>> >>> I do agree that we don’t need to worry about removing synchronized for >>> the purpose of beeing compatible with early versions of Loom (at least not >>> for all commons projects). This is especially true if the code gets more >>> ugly, might have subtle behavior changes or similar. >>> >>> However I think in the context of the PR it looks like the existing code >>> did not use synchronize, so it would be good to not change it to do so >>> (especially not if that’s not needed for the change in question!). >>> >>> I did not follow the changes completely, so I am not sure what’s >>> proposed. Can we we maybe squash it at minimize the changes to fix the >>> actual Bug (if there is one, since I think we still have no specification >>> on concurrency and locking properties of VFS) and keep them Loom support >>> discussion separate from the release? >>> >>> Gruss >>> Bernd >>> -- >>> http://bernd.eckenfels.net >>> >>> Von: Gary Gregory >>> Gesendet: Tuesday, March 2, 2021 7:31:01 PM >>> An: Commons Developers List >>> Betreff: [VFS] Consensus needed for >>> https://github.com/apache/commons-vfs/pull/154 >>> >>> I want to move the discussion from the PR to this mailing list, >>> https://github.com/apache/commons-vfs/pull/154 >>> >>> TY, >>> Gary >>> >>
