mina-ftpserver-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sai Pullabhotla" <sai.pullabho...@jmethods.com>
Subject Re: Question on Ftplets
Date Sun, 16 Nov 2008 15:09:07 GMT
Niklas,

I started changing the code the way we talked and found the following:

The only common place where all FtpReplies go out seems to be from the
FtpIOSession.writeObject() method. So, I updated this method to store
the last reply in an instance variable of FtpIOSession class. Below is
the new writeObject method:

    public WriteFuture write(Object message) {
        WriteFuture future = wrappedSession.write(message);
        this.lastReply = (FtpReply) message;
        return future;
    }


Then in the DefaultFtpHandler, line 144ish, added the new parameter on
the call to ftplet.afterCommand.

                try {
                    ftpletRet = ftplets.afterCommand(session.getFtpletSession(),
                            request, session.getLastReply());
                } catch (Exception e) {
                    LOG.debug("Ftplet container threw exception", e);
                    ftpletRet = FtpletResult.DISCONNECT;
                }

So, the question to you is, do we really want to change the signature
of Ftplet.afterCommand to include the FtpReply parameter or just have
the Ftplet implementations retrieve the last reply from the
FtpSession/FtpIoSession?

Logically, it definitely makes sense to have the parameter, but it is
not required technically. I will wait for your response on this.

Also, for passing additional information about the result of a command
(as discussed in my previous email), I was thinking we should have a
return value from the execute() method of Command class. This return
type could be a ExecutionResult object which can be
implemented/overridden by various commands to provide results with
different parameters. If you like this idea, then need to update the
Ftplet afterCommand method to simply include the ExecutionResult
parameter which tells callers all about the result.

In summary, I think we need to change the
     void execute(FtpIoSession, FtpServerContext, FtpRequest)
to
     ExecutionResult execute(FtpIoSession, FtpServerContext, FtpRequest)

Let me know what you think.

Regards,

Sai Pullabhotla
Phone: (402) 408-5753
Fax: (402) 408-6861
www.jMethods.com




On Sat, Nov 15, 2008 at 2:20 AM, Niklas Gustavsson <niklas@protocol7.com> wrote:
> On Fri, Nov 14, 2008 at 11:35 PM, Sai Pullabhotla
> <sai.pullabhotla@jmethods.com> wrote:
>> What do you think of the following change:
>>
>> Change the signature of Ftplet.afterCommand to -
>>
>> FtpletResult afterCommand(FtpSession session, FtpRequest request,
>> FtpReply reply)
>>            throws FtpException, IOException;
>
> As noted in an earlier mail on this thread, multiple replies might
> have been sent for one request, meaning we need to provide a list of
> replies. Besides that, I like this change.
>
>> I know this breaks the existing applications that are using this
>> interface, but I'm not sure if there is a way to prevent this.
>
> Meaning it has to go into M4 (I hope to have a stable public API in
> M4), so we should have it done quickly.
>
>> Implemntations can then check to see if reply.getCode() and see if it
>> is a Positive Completion Reply (2XX) which means, the requested action
>> was successful. A 4xx or 5xx message means that the requested action
>> has failed.
>>
>> The implementations can then process the parameters of FtpRequest and
>> get any additional details such as file name that the command acted
>> on, its size etc.
>>
>> This would be a good beginning. However, if we want to take it one
>> more step further, It is not a bad idea to create subclasses of
>> DefaultFtpReply to include some more additional information. For e.g.
>> create a new class FtpUploadReply as shown below:
>>
>> public class FtpUploadReply extends DefaultFtpReply {
>>    private FileObject file = null;
>>    private long bytesReceived = 0L; //This may not be needed, as it
>> could be retrieved from the FileObject.
>>    private long transferStartTime = 0L;
>>    private long transferEndTime = 0L;
>> }
>
> We might need to think some further on the details, but I like the
> change in general. However, as this is a non-breaking change, we can
> do this later. For now we should focus on the change above.
>
>> Hope this all makes sense. Let me know what you think and if you would
>> like me to work on this.
>
> Feel free to take a stab. First of all, add JIRA issues describing the
> improvements you would like to see and then we'll attempt to get the
> first change into M4.
>
> /niklas
>

Mime
View raw message