commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Gary D. Gregory (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (VFS-422) Allows to create other channels in SftpFileSystem
Date Thu, 21 Jun 2012 15:20:43 GMT

    [ https://issues.apache.org/jira/browse/VFS-422?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13398482#comment-13398482
] 

Gary D. Gregory commented on VFS-422:
-------------------------------------

Hi Benjamin:

Thank you for providing another patch. 

Let's discuss:

- Meta: The patch format you supplied was painful for me to use. I had to cut and paste individual
fragments for each file and remove the {{a/}} and {{b/}} from the paths. Can you use one plain
old .diff file in the future please? 

- The above may mean I misapplied the patch because running "mvn clean test", I get:

{noformat}
Tests in error:
  testExternalChannel(org.apache.commons.vfs2.provider.sftp.test.ProviderSftpExternalChannelTest)
{noformat}


- Session type: Why is this not an enum? This sure seems like the perfect application of an
enum. The implementation of {{com.jcraft.jsch.CommonsVFSChannelHelper.getChannel(Session,
AtomicLong, String)}} can then be use a switch instead of a bunch of {{if}}s. Also, with enumns,
no chance of a typo creating a bug.

- {{com.jcraft.jsch.CommonsVFSChannelHelper.getChannel(Session, AtomicLong, String)}} 

Why is a runtime exception thrown here:
{code:java}
                @Override
                public void close() {
                    if (!close)
                        counter.decrementAndGet();
                    super.close();
                    throw new RuntimeException("Closed call");
                }
{code}

- Always use blocks, for example:

{code:java}
if (!close) {
   counter.decrementAndGet();
}
{code}

Or, in the case of the goofy VFS style:

{code:java}
if (!close) 
{
   counter.decrementAndGet();
}
{code}

- {{com.jcraft.jsch.CommonsVFSChannelHelper.getChannel(Session, AtomicLong, String)}} instance
initializers

Do we really care to track how many objects are instantiated? Would the increment be better
in connect() overrides? This seems to match up better with the decrement in close(). What
am I missing? Some code comments would help here for the non-Jsch guru.

                
> Allows to create other channels in SftpFileSystem
> -------------------------------------------------
>
>                 Key: VFS-422
>                 URL: https://issues.apache.org/jira/browse/VFS-422
>             Project: Commons VFS
>          Issue Type: Improvement
>    Affects Versions: 2.0
>         Environment: Any
>            Reporter: Benjamin Piwowarski
>         Attachments: 0001-SftpFileSystems-allows-opening-external-channels.patch
>
>   Original Estimate: 3h
>  Remaining Estimate: 3h
>
> In the software I am writing, I need to execute commands when the filesystems "allows"
it, i.e. local or via ssh (i.e. sftp filesystem). 
> For a local filesystem, I can easily do this, but for Sftp, there is no way to get a
channel different than the SFTP on, so it would be great if other channels could be open.
> I could submit a patch that would:
> 1) Allows a call to session.openChannel(String type) [following getChannel()]
> 2) Overwrite isReleaseable to keep the filesystem open if some external channels are
open

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message