You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Gary Gregory <ga...@gmail.com> on 2021/03/02 18:31:01 UTC
[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
Posted by Gary Gregory <ga...@gmail.com>.
Closing the loop on this thread as this PR has been merged.
Gary
On Wed, Mar 3, 2021, 13:06 Gary Gregory <ga...@gmail.com> 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 <ga...@gmail.com> 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 <ec...@zusammenkunft.net>
>> 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 <ga...@gmail.com>
>>> Gesendet: Tuesday, March 2, 2021 7:31:01 PM
>>> An: Commons Developers List <de...@commons.apache.org>
>>> 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
Posted by Gary Gregory <ga...@gmail.com>.
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 <ga...@gmail.com> 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 <ec...@zusammenkunft.net> 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 <ga...@gmail.com>
>> Gesendet: Tuesday, March 2, 2021 7:31:01 PM
>> An: Commons Developers List <de...@commons.apache.org>
>> 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
Posted by Gary Gregory <ga...@gmail.com>.
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 <ec...@zusammenkunft.net> 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 <ga...@gmail.com>
> Gesendet: Tuesday, March 2, 2021 7:31:01 PM
> An: Commons Developers List <de...@commons.apache.org>
> 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
Posted by Bernd Eckenfels <ec...@zusammenkunft.net>.
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 <ga...@gmail.com>
Gesendet: Tuesday, March 2, 2021 7:31:01 PM
An: Commons Developers List <de...@commons.apache.org>
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