From 864177f15584ae5afc1c4ce789399bca29ec54c7 Mon Sep 17 00:00:00 2001 From: pancx Date: Wed, 26 Feb 2025 14:22:02 +0800 Subject: [PATCH] [#6136] improvement(CLI): Move check for enable and disable command in Gravitino CLI to command move check for enable and disable command in ManageMetalake. --- .../gravitino/cli/MetalakeCommandHandler.java | 15 +---- .../gravitino/cli/TestableCommandLine.java | 12 +--- .../gravitino/cli/commands/ManageCatalog.java | 12 ++-- ...etalakeEnable.java => ManageMetalake.java} | 56 +++++++++++++++---- .../cli/commands/MetalakeDisable.java | 56 ------------------- .../gravitino/cli/TestCatalogCommands.java | 3 + .../gravitino/cli/TestMetalakeCommands.java | 28 ++++------ 7 files changed, 69 insertions(+), 113 deletions(-) rename clients/cli/src/main/java/org/apache/gravitino/cli/commands/{MetalakeEnable.java => ManageMetalake.java} (58%) delete mode 100644 clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeDisable.java diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java b/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java index 218f6e14588..c76f875279b 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java @@ -159,19 +159,8 @@ private void handlePropertiesCommand() { /** Handles the "UPDATE" command. */ private void handleUpdateCommand() { - if (line.hasOption(GravitinoOptions.ENABLE) && line.hasOption(GravitinoOptions.DISABLE)) { - System.err.println(ErrorMessages.INVALID_ENABLE_DISABLE); - Main.exit(-1); - } - if (line.hasOption(GravitinoOptions.ENABLE)) { - boolean enableAllCatalogs = line.hasOption(GravitinoOptions.ALL); - gravitinoCommandLine - .newMetalakeEnable(context, metalake, enableAllCatalogs) - .validate() - .handle(); - } - if (line.hasOption(GravitinoOptions.DISABLE)) { - gravitinoCommandLine.newMetalakeDisable(context, metalake).validate().handle(); + if (line.hasOption(GravitinoOptions.ENABLE) || line.hasOption(GravitinoOptions.DISABLE)) { + gravitinoCommandLine.newManageMetalake(context, metalake).validate().handle(); } if (line.hasOption(GravitinoOptions.COMMENT)) { diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java b/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java index 54067b483d0..dcb8e1bc24b 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java @@ -77,10 +77,9 @@ import org.apache.gravitino.cli.commands.ListTopics; import org.apache.gravitino.cli.commands.ListUsers; import org.apache.gravitino.cli.commands.ManageCatalog; +import org.apache.gravitino.cli.commands.ManageMetalake; import org.apache.gravitino.cli.commands.MetalakeAudit; import org.apache.gravitino.cli.commands.MetalakeDetails; -import org.apache.gravitino.cli.commands.MetalakeDisable; -import org.apache.gravitino.cli.commands.MetalakeEnable; import org.apache.gravitino.cli.commands.ModelAudit; import org.apache.gravitino.cli.commands.ModelDetails; import org.apache.gravitino.cli.commands.OwnerDetails; @@ -838,13 +837,8 @@ protected RevokeAllPrivileges newRevokeAllPrivileges( return new RevokeAllPrivileges(context, metalake, role, entity); } - protected MetalakeEnable newMetalakeEnable( - CommandContext context, String metalake, boolean enableAllCatalogs) { - return new MetalakeEnable(context, metalake, enableAllCatalogs); - } - - protected MetalakeDisable newMetalakeDisable(CommandContext context, String metalake) { - return new MetalakeDisable(context, metalake); + protected ManageMetalake newManageMetalake(CommandContext context, String metalake) { + return new ManageMetalake(context, metalake); } protected ManageCatalog newManageCatalog( diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageCatalog.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageCatalog.java index 3ef1b2ed5ea..2e563513916 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageCatalog.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageCatalog.java @@ -23,7 +23,6 @@ import org.apache.gravitino.cli.CommandContext; import org.apache.gravitino.cli.ErrorMessages; import org.apache.gravitino.cli.GravitinoOptions; -import org.apache.gravitino.cli.Main; import org.apache.gravitino.client.GravitinoAdminClient; import org.apache.gravitino.client.GravitinoClient; import org.apache.gravitino.exceptions.MetalakeNotInUseException; @@ -62,21 +61,17 @@ public void handle() { } } - /** - * Validate the command line options. - * - * @return the command instance. - */ + /** {@inheritDoc} */ @Override public Command validate() { if (line.hasOption(GravitinoOptions.ENABLE) && line.hasOption(GravitinoOptions.DISABLE)) { - printInformation(ErrorMessages.INVALID_ENABLE_DISABLE); - Main.exit(-1); + exitWithError(ErrorMessages.INVALID_ENABLE_DISABLE); } return super.validate(); } + /** Enable a catalog. */ private void enableCatalog() { try { if (enableMetalake) { @@ -99,6 +94,7 @@ private void enableCatalog() { printInformation(metalake + "." + catalog + " has been enabled."); } + /** Disable a catalog. */ private void disableCatalog() { try { GravitinoClient client = buildClient(metalake); diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageMetalake.java similarity index 58% rename from clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java rename to clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageMetalake.java index f402c376081..35651838b52 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/ManageMetalake.java @@ -20,34 +20,56 @@ package org.apache.gravitino.cli.commands; import java.util.Arrays; +import org.apache.commons.cli.CommandLine; import org.apache.gravitino.cli.CommandContext; import org.apache.gravitino.cli.ErrorMessages; +import org.apache.gravitino.cli.GravitinoOptions; import org.apache.gravitino.client.GravitinoAdminClient; import org.apache.gravitino.client.GravitinoMetalake; import org.apache.gravitino.exceptions.NoSuchMetalakeException; -/** Enable metalake. */ -public class MetalakeEnable extends Command { - +/** Disable or enable metalake. */ +public class ManageMetalake extends Command { private final String metalake; + private final CommandLine line; private Boolean enableAllCatalogs; /** - * Enable a metalake + * Construct a new instance of the {@code ManageMetalake}. * - * @param context The command context. - * @param metalake The name of the metalake. - * @param enableAllCatalogs Whether to enable all catalogs. + * @param context the command context. + * @param metalake the name of the metalake. */ - public MetalakeEnable(CommandContext context, String metalake, boolean enableAllCatalogs) { + public ManageMetalake(CommandContext context, String metalake) { super(context); this.metalake = metalake; - this.enableAllCatalogs = enableAllCatalogs; + this.line = context.line(); + + this.enableAllCatalogs = line.hasOption(GravitinoOptions.ALL); } - /** Enable metalake. */ + /** Disable or enable the metalake. */ @Override public void handle() { + if (line.hasOption(GravitinoOptions.ENABLE)) { + enableMetalake(); + } else if (line.hasOption(GravitinoOptions.DISABLE)) { + disableMetalake(); + } + } + + /** {@inheritDoc} */ + @Override + public Command validate() { + if (line.hasOption(GravitinoOptions.ENABLE) && line.hasOption(GravitinoOptions.DISABLE)) { + exitWithError(ErrorMessages.INVALID_ENABLE_DISABLE); + } + + return super.validate(); + } + + /** Enable the metalake. */ + private void enableMetalake() { StringBuilder msgBuilder = new StringBuilder(metalake); try { GravitinoAdminClient client = buildAdminClient(); @@ -68,4 +90,18 @@ public void handle() { printInformation(msgBuilder.toString()); } + + /** Disable the metalake. */ + private void disableMetalake() { + try { + GravitinoAdminClient client = buildAdminClient(); + client.disableMetalake(metalake); + } catch (NoSuchMetalakeException err) { + exitWithError(ErrorMessages.UNKNOWN_METALAKE); + } catch (Exception exp) { + exitWithError(exp.getMessage()); + } + + printInformation(metalake + " has been disabled."); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeDisable.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeDisable.java deleted file mode 100644 index ad05d293433..00000000000 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeDisable.java +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, 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 org.apache.gravitino.cli.commands; - -import org.apache.gravitino.cli.CommandContext; -import org.apache.gravitino.cli.ErrorMessages; -import org.apache.gravitino.client.GravitinoAdminClient; -import org.apache.gravitino.exceptions.NoSuchMetalakeException; - -/** Disable metalake. */ -public class MetalakeDisable extends Command { - private String metalake; - - /** - * Disable metalake - * - * @param context The command context. - * @param metalake The name of the metalake. - */ - public MetalakeDisable(CommandContext context, String metalake) { - super(context); - this.metalake = metalake; - } - - /** Disable metalake. */ - @Override - public void handle() { - try { - GravitinoAdminClient client = buildAdminClient(); - client.disableMetalake(metalake); - } catch (NoSuchMetalakeException err) { - exitWithError(ErrorMessages.UNKNOWN_METALAKE); - } catch (Exception exp) { - exitWithError(exp.getMessage()); - } - - printInformation(metalake + " has been disabled."); - } -} diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java index f42059c3d43..553820db1b7 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java @@ -49,6 +49,7 @@ import org.apache.gravitino.cli.commands.UpdateCatalogComment; import org.apache.gravitino.cli.commands.UpdateCatalogName; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -495,5 +496,7 @@ void testCatalogWithDisableAndEnableOptions() { mockCommandLine, mockOptions, CommandEntities.CATALOG, CommandActions.UPDATE)); assertThrows(RuntimeException.class, commandLine::handleCommandLine); + String errorOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errorOutput); } } diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java index e01b652c644..4707d24387e 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java @@ -21,13 +21,11 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.eq; import static org.mockito.Mockito.isNull; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -41,16 +39,15 @@ import org.apache.gravitino.cli.commands.DeleteMetalake; import org.apache.gravitino.cli.commands.ListMetalakeProperties; import org.apache.gravitino.cli.commands.ListMetalakes; +import org.apache.gravitino.cli.commands.ManageMetalake; import org.apache.gravitino.cli.commands.MetalakeAudit; import org.apache.gravitino.cli.commands.MetalakeDetails; -import org.apache.gravitino.cli.commands.MetalakeDisable; -import org.apache.gravitino.cli.commands.MetalakeEnable; import org.apache.gravitino.cli.commands.RemoveMetalakeProperty; import org.apache.gravitino.cli.commands.SetMetalakeProperty; import org.apache.gravitino.cli.commands.UpdateMetalakeComment; import org.apache.gravitino.cli.commands.UpdateMetalakeName; -import org.junit.Assert; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -374,7 +371,7 @@ void testUpdateMetalakeNameForceCommand() { @Test void testEnableMetalakeCommand() { - MetalakeEnable mockEnable = mock(MetalakeEnable.class); + ManageMetalake mockEnable = mock(ManageMetalake.class); when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); when(mockCommandLine.hasOption(GravitinoOptions.ENABLE)).thenReturn(true); @@ -384,7 +381,7 @@ void testEnableMetalakeCommand() { mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); doReturn(mockEnable) .when(commandLine) - .newMetalakeEnable(any(CommandContext.class), eq("metalake_demo"), eq(false)); + .newManageMetalake(any(CommandContext.class), eq("metalake_demo")); doReturn(mockEnable).when(mockEnable).validate(); commandLine.handleCommandLine(); verify(mockEnable).handle(); @@ -392,7 +389,7 @@ void testEnableMetalakeCommand() { @Test void testEnableMetalakeCommandWithRecursive() { - MetalakeEnable mockEnable = mock(MetalakeEnable.class); + ManageMetalake mockEnable = mock(ManageMetalake.class); when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); when(mockCommandLine.hasOption(GravitinoOptions.ALL)).thenReturn(true); @@ -403,7 +400,7 @@ void testEnableMetalakeCommandWithRecursive() { mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); doReturn(mockEnable) .when(commandLine) - .newMetalakeEnable(any(CommandContext.class), eq("metalake_demo"), eq(true)); + .newManageMetalake(any(CommandContext.class), eq("metalake_demo")); doReturn(mockEnable).when(mockEnable).validate(); commandLine.handleCommandLine(); verify(mockEnable).handle(); @@ -411,7 +408,7 @@ void testEnableMetalakeCommandWithRecursive() { @Test void testDisableMetalakeCommand() { - MetalakeDisable mockDisable = mock(MetalakeDisable.class); + ManageMetalake mockDisable = mock(ManageMetalake.class); when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); when(mockCommandLine.hasOption(GravitinoOptions.DISABLE)).thenReturn(true); @@ -422,7 +419,7 @@ void testDisableMetalakeCommand() { mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); doReturn(mockDisable) .when(commandLine) - .newMetalakeDisable(any(CommandContext.class), eq("metalake_demo")); + .newManageMetalake(any(CommandContext.class), eq("metalake_demo")); doReturn(mockDisable).when(mockDisable).validate(); commandLine.handleCommandLine(); verify(mockDisable).handle(); @@ -442,11 +439,8 @@ void testMetalakeWithDisableAndEnableOptions() { new GravitinoCommandLine( mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); - Assert.assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newMetalakeEnable(any(CommandContext.class), eq("metalake_demo"), eq(false)); - verify(commandLine, never()) - .newMetalakeEnable(any(CommandContext.class), eq("metalake_demo"), eq(false)); - assertTrue(errContent.toString().contains(ErrorMessages.INVALID_ENABLE_DISABLE)); + Assertions.assertThrows(RuntimeException.class, commandLine::handleCommandLine); + String errorOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errorOutput); } }