2018-05-31 19:09 GMT+02:00 Lyor Goldstein <[email protected]>:
> I have read the proposed changes and I agree in principle, although the
> suggested re-factoring feels a bit too extensive.
>
> >> remove a few interfaces which are not actually used, i.e. they've been
> introduced because various classes have methods with similar signatures,
> but there's no real concept behind
>
> I disagree with the characterization that they do not have a "real concept
> behind them" they represent contracts of entities that have similar
> attributes. IMO, all the various *XxxHolder*(s) represent an entity that
> provides whatever these "attribute" interfaces hold. This allows for a
> level of abstraction that I find very useful - not to mention enforcing
> certain behavior (as interfaces do). I can go along with perhaps reducing
> some of them, but certainly not most of them. Furthermore, since they are
> very "narrow" interfaces, they allow users to provide an implementation
> that does not "drag" in a bulky object. E.g., if one defines a method as
> accepting a *ClientSessionHolder* it simply tells the user that it will
> need a *ClientSession* - regardless of where it originates - and there are
> many such holders in our code. By removing these "markers" we would be
> forced to un-necessarily ask for "bulkier" objects than necessary. These
> interfaces makes mocking, proxying, overriding behavior etc.. - as well as
> useful containers for default and static helper methods that are common too
> all such holders.
>
Well, I would agree on this principle in theory.
However, I've seen no evidence throughout the code that this is actually
useful.
>
> >> remove the usage of java.nio.channels.Channel which is overused
>
> Agree in principal, but IMO we should keep it for any "pipe" that carries
> information or provides API(s) for it since this is what a *Channel* is
> (IMO). Certainly sessions, channels, streams, SCP/SFTP clients, SFTP opaque
> file handles, etc... satisfy this condition. It is also a very convenient
> standard way to provide a *Closeable* interface + *isOpen()* indicator.
The isOpen() is mostly useless because close() is defined as being
indempotent.
So the call to
if (xx.isOpen()) { xx.close() }
is simply less readable for no gain.
> I do agree though that perhaps it has been used in contexts other than
> "data
> pipes" and in these cases a better definition should be provided - however,
> let's not remove its usage altogether without discriminating between the
> valid cases and the invalid ones.
>
I disagree. I think we should not overextend the meaning of the Channel
interface.
If you look in the JVM, things like Socket, ZipFile, or even InputStream
and OutputStream
do not implement Channel.
This also can lead to confusion with our SSHD related Channel interface
which is
much a more important citizen in our code base.
Could you list the ones that you would see implementing the Channel
interface,
beyond the SftpRemotePathChannel which is an actual FileChannel
implementation ?
>
> >>> introduce an ExecutorService, extending the usual
> java.util.concurrent.ExecutorService and the
> org.apache.sshd.common.Closeable interfaces and refactor all the usage to
> use this interface instead of conveying both the ExecutorService along with
>
> 100% in favor
>
I'll push my changes to a branch tomorrow.
There are a few commits, but most of them, if not very complicated, are
still quite huge
because they affect a lot of classes in the code base.
Guillaume
>
> Cheers,
> Lyor
>
> aaa
>
--
------------------------
Guillaume Nodet
