Patrick Rhomberg commented on GEODE-3584:

A few thoughts, in no particular order:

First, I love the refactoring effort!  So thanks for getting that ball rolling.

This looks like it's going to touch a lot of non-internal, non-private methods 
and classes.  To adhere to SemVer, we need to not break any public-facing API 
until the next major release.  I definitely think these classes could use some 
reorganization, but we should start a conversation on d...@geode.apache.org 
(and maybe user@, too) if we want to, say, deprecate use of the inner-class 
builders in factor of them becoming their own first-class classes, as it were.

It seems like, with how densely connected some of these things are, we should 
probably split this up into several tickets.  I poked through Ken's feature 
branch and took a run at what I had hoped were low-hanging refactor fruits, and 
I immediately realized how non-trivial this effort is going to be.  Potential 
tickets that come to my mind are: several tickets for promoting inner classes 
to their own classes, a ticket for reconciling the differences in the Command 
enums, and another ticket or two to reduce code duplication.

I'll go ahead and open and start the Command reconciliation ticket as a child 
to this ticket.

> Refactor ServerLauncher and LocatorLauncher to eliminate code duplication
> -------------------------------------------------------------------------
>                 Key: GEODE-3584
>                 URL: https://issues.apache.org/jira/browse/GEODE-3584
>             Project: Geode
>          Issue Type: Improvement
>          Components: gfsh
>    Affects Versions: 1.2.0
>            Reporter: Kenneth Howe
>            Priority: Major
>         Attachments: GEODE-3584-1, GEODE-3584-WIP, GEODE-3584-WIP-2, 
> GEODE-3584-WIP-3, GEODE-3584-WIP-4
> There is some duplication of code in the Launcher classes that can be 
> eliminated. Both classes have methods such as getBindAddress (called 
> getServerBindAddress in ServerLauncher) that could be hoisted into  
> AbstractLauncher class that they both extend. The same goes for the inner 
> State classes of the Launchers; they have methods that could be moved to 
> AbstractLauncher.ServiceState.

This message was sent by Atlassian JIRA

Reply via email to