Feature/3.x refactor #764
Feature/3.x refactor #764
Conversation
# Conflicts: # marklogic-data-hub/src/test/java/com/marklogic/hub/EntityManagerTest.java
…ing/final databases had the resource loaded
…cies on unsupporting libraries for hub config and remove the json ignore for the interface exposed methods
Refactor install info
|
|
||
| protected final Logger logger = LoggerFactory.getLogger(this.getClass()); | ||
| //HubDatabase stuff goes here | ||
| enum HubDatabase { |
There was a problem hiding this comment.
This seems redundant with DatabaseKind, except that this enum has only staging and final. Does it make sense to have both?
| logger.info("Initializing the Hub Project"); | ||
| hubConfig.initHubProject(); | ||
| } | ||
| void initProject(); |
There was a problem hiding this comment.
Please add a javadoc comment saying what this does.
| for (Resource r : resolver.getResources("classpath*:/ml-modules-jobs/options/*.xml")) { | ||
| options.add(r.getFilename().replace(".xml", "")); | ||
| } | ||
| void clearUserModules(); |
There was a problem hiding this comment.
The TODO in the comment doesn't belong here anymore.
| if (check.finalPortInUse) { | ||
| check.finalPortInUseBy = serverName; | ||
| } | ||
| void runPreInstallCheck(); |
There was a problem hiding this comment.
Seems odd that this is void. What happens as a result of calling this method? Will I get an exception if the check fails? What does it mean to fail?
There was a problem hiding this comment.
Nope, it only populates things. The install process is disconnected a bit
| check.serverVersionOk = isServerVersionValid(check.serverVersion); | ||
| return check; | ||
| } | ||
| void runPreInstallCheck(Versions versions); |
|
|
||
| /** | ||
| * Removes user's modules from the modules db | ||
| * TODO: this becomes much simpler when we move code into the server dir |
There was a problem hiding this comment.
can we remove this TODO from the comment now?
| } | ||
|
|
||
| /** | ||
| * Installs the data hub configuration and server-side modules into MarkLogic |
There was a problem hiding this comment.
remove server-side modules reference?
| } | ||
|
|
||
| /** | ||
| * Installs the data hub configuration and server-side modules into MarkLogic |
There was a problem hiding this comment.
remove server-side modules reference?
|
|
||
| private static final String GRADLE_PROPERTIES_FILENAME = "gradle.properties"; | ||
|
|
||
| private HubConfigImpl hubConfig; |
There was a problem hiding this comment.
Not sure about accepted best practice, but I'm inclined to use HubConfig here instead of HubConfigImpl, unless there's a compelling reason to do otherwise. Do we need to use the Impl class?
There was a problem hiding this comment.
I agree, unless there's some access to non-interface methods in here.
There was a problem hiding this comment.
We do to encapsulate our reliance on the mgmt library we use to manage clients from being exposed through the interface.
| @@ -186,7 +183,7 @@ private void installUserModules(HubConfig hubConfig, boolean forceLoad, DeployUs | |||
| loadUserModulesCommand.setForceLoad(forceLoad); | |||
| commands.add(loadUserModulesCommand); | |||
|
|
|||
| SimpleAppDeployer deployer = new SimpleAppDeployer(hubConfig.getManageClient(), hubConfig.getAdminManager()); | |||
| SimpleAppDeployer deployer = new SimpleAppDeployer(((HubConfigImpl)hubConfig).getManageClient(), ((HubConfigImpl)hubConfig).getAdminManager()); | |||
There was a problem hiding this comment.
I think we should be using HubConfig here, not HubConfigImpl.
There was a problem hiding this comment.
Exposes implementation of the manageclient & other mgmt library methods. Direction is to remove or encapsulate any dependencies that aren't directly supported.
This exposes the impl class to our datahubservice, which relies on the non-interface methods provided by the hubconfigimpl. A better way would be to provide OOTB a way to deploy user code from datahub interface that takes a hubconfig object as a parameter. that is then called by this private class.
…ng methods based off that
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package com.marklogic.hub; |
There was a problem hiding this comment.
We do have to include this copyright and apache message in each file.
|
|
||
| private static final String GRADLE_PROPERTIES_FILENAME = "gradle.properties"; | ||
|
|
||
| private HubConfigImpl hubConfig; |
There was a problem hiding this comment.
I agree, unless there's some access to non-interface methods in here.
| throw new InvalidDBOperationError(kind, "test appserver existence"); | ||
| } | ||
| return exists; | ||
| } |
There was a problem hiding this comment.
begs the question whether the databases should all be in a HashMap<DatabaseKind,DatabasClient> of course that can be a future refactor too
There was a problem hiding this comment.
Ideally attached and managed from in hubconfig, as well.
No description provided.