You are viewing a plain text version of this content. The canonical link for it is here.
Posted to user@hive.apache.org by Edward Capriolo <ed...@gmail.com> on 2009/09/30 04:42:40 UTC
Should all processors return a DriverResponse?
All,
I am looking to integrate HWI with
https://issues.apache.org/jira/browse/HIVE-795
Should all Processors return a DriverResponse? the web interface
allows a list of commands as the CLI would take.
I was storing this in List<Int>
I was looking to change this to List<DriverResult>
I also have to extend the class...
class DriverResponseWrapper extends DriverResponse {
public DriverResponseWrapper (int x){
super(x);
}
}
Should DriverReponse and CommandResponse exist maybe as a subclass of Response.
Edward
Re: Should all processors return a DriverResponse?
Posted by Bill Graham <bi...@gmail.com>.
Edward your approach looks good, but I have a few comments.
- Since we agree that we don't need to worry about backward compatibility,
then why have both methods as part of the CommandProcessor interface? Seems
we should get rid of the method that returns an int. If we decide to keep
it, then we're saying that backward compatibility does matter to us. In that
case we should keep it and deprecate it.
- You included SQLState in the response base class, which makes sense. If
that's the case though, do we need to create DriverResponse and
CommandResponse subclasses? If the subclasses need to have add'l methods,
then yes we do. But if they don't then I don't see the need. We could always
subclass later if the need arises. Maybe we extend for DriverResponse just
for backward compatibility, but otherwise we don't need to?
thanks,
Bill
On Wed, Sep 30, 2009 at 1:45 PM, Edward Capriolo <ed...@gmail.com>wrote:
> On Wed, Sep 30, 2009 at 4:32 PM, Bill Graham <bi...@gmail.com> wrote:
> > Looking again at the solution to HIVE-795 I see that adding the
> runCommand
> > method to Driver
> > worked for that class, but deviated from the approach used by the
> > CommandProcessor interface. Hence the issue you're running into.
> >
> > Driver got this new method:
> >
> > public DriverResponse runCommand(String command)
> >
> > But the CommandProcessor interface, which Driver implements has this
> method:
> >
> > public int run(String command)
> >
> >
> > Other implementations CommandProcessor should also return a composite
> > response object instead of an int. I think the ideal solution would be if
> > the CommandProcessor interface instead had a method either of these:
> >
> > public CommandProcessorResponse runCommand(String command);
> >
> > or
> >
> > public CommandProcessorResponse run(String command);
> >
> > And CommandProcessorResponse had attributes for responseCode and
> > errorMessage. DriverResponse could extend CommandProcessorResponse (as
> could
> > other response types as needed) and have the SQLState attribute.
> >
> > If we move towards a solution like this, then the question becomes how
> hard
> > to we try to maintain backward compatibility with the CommandProcessor
> > interface? Do we just change the interface (which would be easier and
> result
> > in cleaner code) or do we migrate to a new interface and deprecate the
> old?
> > I lean towards the former, but would like to hear what others have to
> say.
> > Although it's a public method, I'd expect that there probably aren't many
> > implementations outside of the Hive code base that are written against
> > CommandProcessor, and the fact that we're at a 0.0.x version should imply
> > that internal interfaces might change from release to release. Other
> > thoughts?
> >
> > thanks,
> > Bill
> >
> >
> > On Tue, Sep 29, 2009 at 7:42 PM, Edward Capriolo <ed...@gmail.com>
> > wrote:
> >>
> >> All,
> >>
> >> I am looking to integrate HWI with
> >> https://issues.apache.org/jira/browse/HIVE-795
> >> Should all Processors return a DriverResponse? the web interface
> >> allows a list of commands as the CLI would take.
> >>
> >> I was storing this in List<Int>
> >>
> >> I was looking to change this to List<DriverResult>
> >>
> >> I also have to extend the class...
> >>
> >> class DriverResponseWrapper extends DriverResponse {
> >> public DriverResponseWrapper (int x){
> >> super(x);
> >> }
> >> }
> >>
> >> Should DriverReponse and CommandResponse exist maybe as a subclass of
> >> Response.
> >>
> >> Edward
> >
> >
>
> Thanks Bill,
>
> I wanted to open a Jira but it seems like an issue the past two days.
> I agree that there are few/no implementations outside of the CLI of
> CommandProcessor. I do not think we need to support backwards
> compatibility for the change, but doing it like DriverResponse is
> logical.
>
> Have the old method be a chained call to the new method
>
> I have a slight variation of your suggestion but essentially the same idea.
>
> Driver.java
> int run (String command)
> DriverResponse runCommand(command)
>
>
> We should do the same with CommandProcessor
> CommandProcessor.java
> int run (String command)
> CommandResponse runCommand(command)
>
> The refactoring would involved creating an abstract base class
> Response. All the private members would move to the base class and
> become protected.
>
> public abstract class Response {
> protected int responseCode;
> protected String errorMessage;
> protected String SQLState;
>
> public DriverResponse(int responseCode) {
> this(responseCode, null, null);
> }
>
> public DriverResponse(int responseCode, String errorMessage,
> String SQLState) {
> this.responseCode = responseCode;
> this.errorMessage = errorMessage;
> this.SQLState = SQLState;
> }
>
> public int getResponseCode() { return responseCode; }
> public String getErrorMessage() { return errorMessage; }
> public String getSQLState() { return SQLState; }
> }
>
>
> public class DriverResponse extends Response{
>
> public DriverResponse(int responseCode) {
> this(responseCode, null, null);
> }
>
> public DriverResponse(int responseCode, String errorMessage,
> String SQLState) {
> super(responseCode,errorMessage,SQLState);
> }
> }
>
> public class CommandResponse extends Response{
>
> public CommandResponse(int responseCode) {
> this(responseCode, null, null);
> }
>
> public CommandResponse(int responseCode, String errorMessage,
> String SQLState) {
> super(responseCode,errorMessage,SQLState);
> }
> }
>
Re: Should all processors return a DriverResponse?
Posted by Edward Capriolo <ed...@gmail.com>.
On Wed, Sep 30, 2009 at 4:32 PM, Bill Graham <bi...@gmail.com> wrote:
> Looking again at the solution to HIVE-795 I see that adding the runCommand
> method to Driver
> worked for that class, but deviated from the approach used by the
> CommandProcessor interface. Hence the issue you're running into.
>
> Driver got this new method:
>
> public DriverResponse runCommand(String command)
>
> But the CommandProcessor interface, which Driver implements has this method:
>
> public int run(String command)
>
>
> Other implementations CommandProcessor should also return a composite
> response object instead of an int. I think the ideal solution would be if
> the CommandProcessor interface instead had a method either of these:
>
> public CommandProcessorResponse runCommand(String command);
>
> or
>
> public CommandProcessorResponse run(String command);
>
> And CommandProcessorResponse had attributes for responseCode and
> errorMessage. DriverResponse could extend CommandProcessorResponse (as could
> other response types as needed) and have the SQLState attribute.
>
> If we move towards a solution like this, then the question becomes how hard
> to we try to maintain backward compatibility with the CommandProcessor
> interface? Do we just change the interface (which would be easier and result
> in cleaner code) or do we migrate to a new interface and deprecate the old?
> I lean towards the former, but would like to hear what others have to say.
> Although it's a public method, I'd expect that there probably aren't many
> implementations outside of the Hive code base that are written against
> CommandProcessor, and the fact that we're at a 0.0.x version should imply
> that internal interfaces might change from release to release. Other
> thoughts?
>
> thanks,
> Bill
>
>
> On Tue, Sep 29, 2009 at 7:42 PM, Edward Capriolo <ed...@gmail.com>
> wrote:
>>
>> All,
>>
>> I am looking to integrate HWI with
>> https://issues.apache.org/jira/browse/HIVE-795
>> Should all Processors return a DriverResponse? the web interface
>> allows a list of commands as the CLI would take.
>>
>> I was storing this in List<Int>
>>
>> I was looking to change this to List<DriverResult>
>>
>> I also have to extend the class...
>>
>> class DriverResponseWrapper extends DriverResponse {
>> public DriverResponseWrapper (int x){
>> super(x);
>> }
>> }
>>
>> Should DriverReponse and CommandResponse exist maybe as a subclass of
>> Response.
>>
>> Edward
>
>
Thanks Bill,
I wanted to open a Jira but it seems like an issue the past two days.
I agree that there are few/no implementations outside of the CLI of
CommandProcessor. I do not think we need to support backwards
compatibility for the change, but doing it like DriverResponse is
logical.
Have the old method be a chained call to the new method
I have a slight variation of your suggestion but essentially the same idea.
Driver.java
int run (String command)
DriverResponse runCommand(command)
We should do the same with CommandProcessor
CommandProcessor.java
int run (String command)
CommandResponse runCommand(command)
The refactoring would involved creating an abstract base class
Response. All the private members would move to the base class and
become protected.
public abstract class Response {
protected int responseCode;
protected String errorMessage;
protected String SQLState;
public DriverResponse(int responseCode) {
this(responseCode, null, null);
}
public DriverResponse(int responseCode, String errorMessage,
String SQLState) {
this.responseCode = responseCode;
this.errorMessage = errorMessage;
this.SQLState = SQLState;
}
public int getResponseCode() { return responseCode; }
public String getErrorMessage() { return errorMessage; }
public String getSQLState() { return SQLState; }
}
public class DriverResponse extends Response{
public DriverResponse(int responseCode) {
this(responseCode, null, null);
}
public DriverResponse(int responseCode, String errorMessage,
String SQLState) {
super(responseCode,errorMessage,SQLState);
}
}
public class CommandResponse extends Response{
public CommandResponse(int responseCode) {
this(responseCode, null, null);
}
public CommandResponse(int responseCode, String errorMessage,
String SQLState) {
super(responseCode,errorMessage,SQLState);
}
}
Re: Should all processors return a DriverResponse?
Posted by Bill Graham <bi...@gmail.com>.
Looking again at the solution to HIVE-795 I see that adding the runCommand
method to Driver
worked for that class, but deviated from the approach used by the
CommandProcessor interface. Hence the issue you're running into.
Driver got this new method:
public DriverResponse runCommand(String command)
But the CommandProcessor interface, which Driver implements has this method:
public int run(String command)
Other implementations CommandProcessor should also return a composite
response object instead of an int. I think the ideal solution would be if
the CommandProcessor interface instead had a method either of these:
public CommandProcessorResponse runCommand(String command);
or
public CommandProcessorResponse run(String command);
And CommandProcessorResponse had attributes for responseCode and
errorMessage. DriverResponse could extend CommandProcessorResponse (as could
other response types as needed) and have the SQLState attribute.
If we move towards a solution like this, then the question becomes how hard
to we try to maintain backward compatibility with the CommandProcessor
interface? Do we just change the interface (which would be easier and result
in cleaner code) or do we migrate to a new interface and deprecate the old?
I lean towards the former, but would like to hear what others have to say.
Although it's a public method, I'd expect that there probably aren't many
implementations outside of the Hive code base that are written against
CommandProcessor, and the fact that we're at a 0.0.x version should imply
that internal interfaces might change from release to release. Other
thoughts?
thanks,
Bill
On Tue, Sep 29, 2009 at 7:42 PM, Edward Capriolo <ed...@gmail.com>wrote:
> All,
>
> I am looking to integrate HWI with
> https://issues.apache.org/jira/browse/HIVE-795
> Should all Processors return a DriverResponse? the web interface
> allows a list of commands as the CLI would take.
>
> I was storing this in List<Int>
>
> I was looking to change this to List<DriverResult>
>
> I also have to extend the class...
>
> class DriverResponseWrapper extends DriverResponse {
> public DriverResponseWrapper (int x){
> super(x);
> }
> }
>
> Should DriverReponse and CommandResponse exist maybe as a subclass of
> Response.
>
> Edward
>