You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "PakhomovAlexander (via GitHub)" <gi...@apache.org> on 2023/06/08 12:47:45 UTC
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #2152: IGNITE-19522 Modify deploy unit command
PakhomovAlexander commented on code in PR #2152:
URL: https://github.com/apache/ignite-3/pull/2152#discussion_r1222973642
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/Options.java:
##########
@@ -240,6 +241,15 @@ public static final class Constants {
/** Unit path option description. */
public static final String UNIT_PATH_OPTION_DESC = "Path to deployment unit file or directory";
+ /** Unit nodes option long name. */
+ public static final String UNIT_NODES_OPTION = "--nodes";
+
+ /** Unit nodes option short name. */
+ public static final String UNIT_NODES_OPTION_SHORT = "-un";
Review Comment:
if I am not mistaken, we decided not to use the two-letter symbols for short options. It might be confusing to understand if it means -u -n or -un.
##########
modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/unit/ItDeploymentUnitTest.java:
##########
@@ -156,6 +156,98 @@ void deployDirectory() {
);
}
+ @Test
+ @DisplayName("Should display correct status after deploy to the specified nodes")
+ void deployToNodesAndStatusCheck() {
+ // When deploy with version
+ String node = allNodeNames().get(1);
+ execute("cluster", "unit", "deploy", "test.unit.id.7", "--version", "1.0.0", "--path", testFile, "--nodes", node);
+
+ // Then
+ assertAll(
+ this::assertExitCodeIsZero,
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("Done")
+ );
+
+ await().untilAsserted(() -> {
+ resetOutput();
+ execute("cluster", "unit", "list", "test.unit.id.7");
+
+ // Unit is deployed on all requested nodes
+ assertAll(
+ this::assertExitCodeIsZero,
+ this::assertErrOutputIsEmpty,
+ () -> assertOutputContains("1.0.0"),
+ () -> assertOutputContains("DEPLOYED")
+ );
+
+ resetOutput();
+ execute("node", "unit", "list", "--node-url", "http://localhost:10301", "test.unit.id.7");
Review Comment:
could you check that it is also deployed to the CMG group?
##########
modules/code-deployment/src/main/java/org/apache/ignite/internal/deployunit/IgniteDeployment.java:
##########
@@ -28,29 +28,80 @@
*/
public interface IgniteDeployment extends IgniteComponent {
/**
- * Deploy provided unit to current node.
- * After deploy finished, this deployment unit will be place to CMG group asynchronously.
+ * Deploy provided unit to the current node. After the deploy is finished, the unit will be placed to the CMG group, if
+ * {@code deployMode} is {@code MAJORITY}, or to all available units, if {@code deployMode} is {@code ALL} asynchronously.
*
* @param id Unit identifier. Not empty and not null.
* @param version Unit version.
* @param deploymentUnit Unit content.
+ * @param deployMode Initial deploy mode.
* @return Future with success or not result.
*/
- default CompletableFuture<Boolean> deployAsync(String id, Version version, DeploymentUnit deploymentUnit) {
- return deployAsync(id, version, false, deploymentUnit);
+ default CompletableFuture<Boolean> deployAsync(
+ String id,
+ Version version,
+ DeploymentUnit deploymentUnit,
+ InitialDeployMode deployMode
+ ) {
+ return deployAsync(id, version, false, deploymentUnit, deployMode);
}
/**
- * Deploy provided unit to current node.
- * After deploy finished, this deployment unit will be place to CMG group asynchronously.
+ * Deploy provided unit to the current node. After the deploy is finished, the unit will be placed to the CMG group, if
+ * {@code deployMode} is {@code MAJORITY}, or to all available units, if {@code deployMode} is {@code ALL} asynchronously.
*
* @param id Unit identifier. Not empty and not null.
* @param version Unit version.
* @param force Force redeploy if unit with provided id and version exists.
* @param deploymentUnit Unit content.
+ * @param deployMode Initial deploy mode.
* @return Future with success or not result.
*/
- CompletableFuture<Boolean> deployAsync(String id, Version version, boolean force, DeploymentUnit deploymentUnit);
+ CompletableFuture<Boolean> deployAsync(
+ String id,
+ Version version,
+ boolean force,
+ DeploymentUnit deploymentUnit,
+ InitialDeployMode deployMode
+ );
+
+ /**
+ * Deploy provided unit to the current node. After the deploy is finished, the unit will be placed to the CMG group and to the nodes
+ * passed in the @{code initialNodes} list asynchronously.
+ *
+ * @param id Unit identifier. Not empty and not null.
+ * @param version Unit version.
+ * @param deploymentUnit Unit content.
+ * @param initialNodes List of nodes to deploy to initially.
+ * @return Future with success or not result.
+ */
+ default CompletableFuture<Boolean> deployAsync(
+ String id,
+ Version version,
+ DeploymentUnit deploymentUnit,
+ List<String> initialNodes
Review Comment:
Maybe just `nodes` is enogh?
##########
modules/code-deployment/src/main/java/org/apache/ignite/internal/deployunit/DeploymentManagerImpl.java:
##########
@@ -151,32 +155,72 @@ private void onUnitRegister(UnitNodeStatus status, Set<String> deployedNodes) {
}
@Override
- public CompletableFuture<Boolean> deployAsync(String id, Version version, boolean force, DeploymentUnit deploymentUnit) {
+ public CompletableFuture<Boolean> deployAsync(
+ String id,
+ Version version,
+ boolean force,
+ DeploymentUnit deploymentUnit,
+ InitialDeployMode deployMode
+ ) {
checkId(id);
Objects.requireNonNull(version);
Objects.requireNonNull(deploymentUnit);
- return cmgManager.cmgNodes()
- .thenCompose(cmg -> deploymentUnitStore.createClusterStatus(id, version, cmg))
+ return extractNodes(deployMode)
+ .thenCompose(nodesToDeploy ->
+ doDeploy(id, version, force, deploymentUnit, nodesToDeploy,
+ undeployed -> deployAsync(id, version, deploymentUnit, deployMode)
+ )
+ );
+ }
+
+ @Override
+ public CompletableFuture<Boolean> deployAsync(
+ String id,
+ Version version,
+ boolean force,
+ DeploymentUnit deploymentUnit,
+ List<String> initialNodes
+ ) {
+ checkId(id);
+ Objects.requireNonNull(version);
+ Objects.requireNonNull(deploymentUnit);
+
+ return extractNodes(initialNodes)
+ .thenCompose(nodesToDeploy ->
+ doDeploy(id, version, force, deploymentUnit, nodesToDeploy,
+ undeployed -> deployAsync(id, version, deploymentUnit, initialNodes)
Review Comment:
Looks like a recursion, am I right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org