[
https://issues.apache.org/jira/browse/GEODE-3584?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16304277#comment-16304277
]
Cyrille Artho edited comment on GEODE-3584 at 12/27/17 6:49 AM:
----------------------------------------------------------------
Hi,
I can help with this task. As far as I can see, the following fields are used
in the three classes in question (see table). I tried to put fields with the
same name and type side by side, sometimes adjacent to non-matching fields of
the base class to save space.
Fields in bold are exact duplicates in both subclasses, and those in bold +
italics are close enough that I think they can be merged.
I can start with exact matches and then try to merge some non-exact matches.
Some questions before I start:
# Does anyone know why some fields are declared "transient" (not to be
serialized) in one case but not in the other?
# Is there a way to measure test coverage with the given unit tests?
# Would you prefer one large patch with many merges, or several smaller ones?
# What's the fastest way to run only the tests for the classes that changed? In
this case, I do not expect to change any classes outside the three affected by
this issue.
If I do not get a response, I will start by merging all the identical fields.
To ensure that at least one test covers a field and its access methods, I'll
try to locate a test that crashes if I break some of the access methods on
temporarily. If no test covers that field, I'll add one.
||AbstractLauncher.java||LocatorLauncher.java||ServerLauncher.java||
|static final Boolean DEFAULT_FORCE|static final Boolean
DEFAULT_LOAD_SHARED_CONFIG_FROM_DIR|static final ServerLauncherCacheProvider
DEFAULT_CACHE_PROVIDER|
|static final long READ_PID_FILE_TIMEOUT_MILLIS|*static final Map<String,
String> helpMap = new HashMap<>()*|*static final Map<String, String> helpMap =
new HashMap<>()*|
|static final String DEFAULT_WORKING_DIRECTORY|*static final Map<Command,
String> usageMap = new TreeMap<>()*|*static final Map<Command, String> usageMap
= new TreeMap<>()*|
|static final String SIGNAL_HANDLER_REGISTRATION_SYSTEM_PROPERTY|*_static final
String DEFAULT_LOCATOR_LOG_EXT = ".log"_*|*_static final String
DEFAULT_SERVER_LOG_EXT = ".log"_*|
|static final String OPTION_PREFIX|*_static final String
DEFAULT_LOCATOR_LOG_NAME = "locator"_*|*_static final String
DEFAULT_SERVER_LOG_NAME = "gemfire"_*|
|static final String SUN_SIGNAL_API_CLASS_NAME|*_static final String
LOCATOR_SERVICE_NAME = "Locator"_*|*_static final String SERVER_SERVICE_NAME =
"Server"_*|
| |*_static final AtomicReference<LocatorLauncher> INSTANCE = new
AtomicReference<>()_*|*_static final AtomicReference<ServerLauncher> INSTANCE =
new AtomicReference<>()_*|
|*volatile boolean debug*| |*volatile boolean debug*|
| |*_final transient ControlNotificationHandler controlHandler_*|*_final
ControlNotificationHandler controlHandler_*|
|final transient AtomicBoolean running = new AtomicBoolean(false)| | |
| |*final AtomicBoolean starting = new AtomicBoolean(false)*|*final
AtomicBoolean starting = new AtomicBoolean(false)*|
|Logger logger = Logger.getLogger(getClass().getName())| |final boolean
assignBuckets|
| |*final boolean deletePidFileOnStop*|*final boolean deletePidFileOnStop*|
| | |final boolean disableDefaultServer|
| |*final boolean force*|*final boolean force*|
| |*final boolean help*|*final boolean help*|
| | |final boolean rebalance|
| |*final boolean redirectOutput*|*final boolean redirectOutput*|
| | |volatile Cache cache|
| | |final CacheConfig cacheConfig|
| |*final Command command*|*final Command command*|
| |*_final boolean bindAddressSpecified_*|*_final InetAddress
serverBindAddress_*|
| |*final Integer pid*|*final Integer pid*|
| |*_final boolean portSpecified_*|*_final Integer serverPort_*|
| |*final Properties distributedSystemProperties*|*final Properties
distributedSystemProperties*|
| |*final String memberName*|*final String memberName*|
| |final Integer port|final String springXmlLocation|
| |*final String workingDirectory*|*final String workingDirectory*|
| |*_transient volatile String statusMessage_*|*_volatile String
statusMessage_*|
| | |final Float criticalHeapPercentage|
| | |final Float evictionHeapPercentage|
| | |final Float criticalOffHeapPercentage|
| | |final Float evictionOffHeapPercentage|
| |*final String hostnameForClients*|*final String hostNameForClients*|
| | |final Integer maxConnections|
| | |final Integer maxMessageCount|
| | |final Integer messageTimeToLive|
| | |final Integer socketBufferSize|
| | |final Integer maxThreads|
| |*_transient volatile ControllableProcess process_*|*_volatile
ControllableProcess process_*|
| |*_final transient LocatorControllerParameters controllerParameters_*|*_final
ServerControllerParameters controllerParameters_*|
| |transient volatile InternalLocator locator| |
| |final boolean workingDirectorySpecified| |
| |final InetAddress bindAddress| |
was (Author: telcontar):
Hi,
I can help with this task. As far as I can see, the following fields are used
in the three classes in question (see table). I tried to put fields with the
same name and type side by side, sometimes adjacent to non-matching fields of
the base class to save space.
Fields in bold are exact duplicates in both subclasses, and those in bold +
italics are close enough that I think they can be merged.
I can start with exact matches and then try to merge some non-exact matches.
Some questions before I start:
# Does anyone know why some fields are declared "transient" (not to be
serialized) in one case but not in the other?
# Is there a way to measure test coverage with the given unit tests?
# Would you prefer one large patch with many merges, or several smaller ones?
# What's the fastest way to run only the tests for the classes that changed? In
this case, I do not expect to change any classes outside the three affected by
this issue.
If I do not get a response, I will start by merging all the identical fields.
To ensure that at least one test covers a field and its access methods, I'll
try to locate a test that crashes if I break some of the access methods on
temporarily. If no test covers that field, I'll add one.
||AbstractLauncher.java||LocatorLauncher.java||ServerLauncher.java||
|static final Boolean DEFAULT_FORCE|static final Boolean
DEFAULT_LOAD_SHARED_CONFIG_FROM_DIR|static final ServerLauncherCacheProvider
DEFAULT_CACHE_PROVIDER|
|static final long READ_PID_FILE_TIMEOUT_MILLIS|*static final Map<String,
String> helpMap = new HashMap<>()*|*static final Map<String, String> helpMap =
new HashMap<>()*|
|static final String DEFAULT_WORKING_DIRECTORY|*static final Map<Command,
String> usageMap = new TreeMap<>()*|*static final Map<Command, String> usageMap
= new TreeMap<>()*|
|static final String SIGNAL_HANDLER_REGISTRATION_SYSTEM_PROPERTY|*_static final
String DEFAULT_LOCATOR_LOG_EXT = ".log"_*|*_static final String
DEFAULT_SERVER_LOG_EXT = ".log"_*|
|static final String OPTION_PREFIX|*_static final String
DEFAULT_LOCATOR_LOG_NAME = "locator"_*|*_static final String
DEFAULT_SERVER_LOG_NAME = "gemfire"_*|
|static final String SUN_SIGNAL_API_CLASS_NAME|*_static final String
LOCATOR_SERVICE_NAME = "Locator"_*|*_static final String SERVER_SERVICE_NAME =
"Server"_*|
| |*_static final AtomicReference<LocatorLauncher> INSTANCE = new
AtomicReference<>()_*|*_static final AtomicReference<ServerLauncher> INSTANCE =
new AtomicReference<>()_*|
|*volatile boolean debug*| |*volatile boolean debug*|
| |*_final transient ControlNotificationHandler controlHandler_*|*_final
ControlNotificationHandler controlHandler_*|
|final transient AtomicBoolean running = new AtomicBoolean(false)| | |
| |*final AtomicBoolean starting = new AtomicBoolean(false)*|*final
AtomicBoolean starting = new AtomicBoolean(false)*|
|Logger logger = Logger.getLogger(getClass().getName())| |final boolean
assignBuckets|
| |*final boolean deletePidFileOnStop*|*final boolean deletePidFileOnStop*|
| | |final boolean disableDefaultServer|
| |*final boolean force*|*final boolean force*|
| |*final boolean help*|*final boolean help*|
| | |final boolean rebalance|
| |*final boolean redirectOutput*|*final boolean redirectOutput*|
| | |volatile Cache cache|
| | |final CacheConfig cacheConfig|
| |*final Command command*|*final Command command*|
| |*_final boolean bindAddressSpecified_*|*_final InetAddress
serverBindAddress_*|
| |*final Integer pid*|*final Integer pid*|
| |*_final boolean portSpecified_*|*_final Integer serverPort_*|
| |*final Properties distributedSystemProperties*|*final Properties
distributedSystemProperties*|
| |*final String memberName*|*final String memberName*|
| |final Integer port|final String springXmlLocation|
| |*final String workingDirectory*|*final String workingDirectory*|
| |*_transient volatile String statusMessage_*|*_volatile String
statusMessage_*|
| | |final Float criticalHeapPercentage|
| | |final Float evictionHeapPercentage|
| | |final Float criticalOffHeapPercentage|
| | |final Float evictionOffHeapPercentage|
| |*final String hostnameForClients*|*final String hostNameForClients*|
| | |final Integer maxConnections|
| | |final Integer maxMessageCount|
| | |final Integer messageTimeToLive|
| | |final Integer socketBufferSize|
| | |final Integer maxThreads|
| |*_transient volatile ControllableProcess process_*|*_volatile
ControllableProcess proces_*s|
| |*_final transient LocatorControllerParameters controllerParameters_*|*_final
ServerControllerParameters controllerParameters_*|
| |transient volatile InternalLocator locator| |
| |final boolean workingDirectorySpecified| |
| |final InetAddress bindAddress| |
> 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
>
> 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
(v6.4.14#64029)
