You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@geode.apache.org by "Cyrille Artho (JIRA)" <ji...@apache.org> on 2017/12/27 06:50:00 UTC
[jira] [Comment Edited] (GEODE-3584) Refactor ServerLauncher and
LocatorLauncher to eliminate code duplication
[ 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)