[VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154

garydgregory
I want to move the discussion from the PR to this mailing list,
https://github.com/apache/commons-vfs/pull/154

TY,
Gary
Reply | Threaded
Open this post in threaded view
|

Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154

Bernd Eckenfels
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 <[hidden email]>
Gesendet: Tuesday, March 2, 2021 7:31:01 PM
An: Commons Developers List <[hidden email]>
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
Reply | Threaded
Open this post in threaded view
|

Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154

garydgregory
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 <[hidden email]> 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 <[hidden email]>
> Gesendet: Tuesday, March 2, 2021 7:31:01 PM
> An: Commons Developers List <[hidden email]>
> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154

garydgregory
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>> Gesendet: Tuesday, March 2, 2021 7:31:01 PM
>> An: Commons Developers List <[hidden email]>
>> 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
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [VFS] Consensus needed for https://github.com/apache/commons-vfs/pull/154

garydgregory
Closing the loop on this thread as this PR has been merged.

Gary

On Wed, Mar 3, 2021, 13:06 Gary Gregory <[hidden email]> 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 <[hidden email]> 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 <[hidden email]>
>> 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 <[hidden email]>
>>> Gesendet: Tuesday, March 2, 2021 7:31:01 PM
>>> An: Commons Developers List <[hidden email]>
>>> 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
>>>
>>