You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/12/07 22:24:03 UTC
[GitHub] [hadoop] omalley opened a new pull request, #5195: HDFS-16856: Refactor RouterAdmin to use the AdminHelper class.
omalley opened a new pull request, #5195:
URL: https://github.com/apache/hadoop/pull/5195
### Description of PR
This simplifies the structure of RouterAdmin to make the various subcommands, much more consistent and have less repetition throughout the code. The user visible change is that some of the error and help messages have changed and in general are more consistent.
### How was this patch tested?
The unit test TestRouterAdminCLI was run. I'll also test the tool against our routers.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] goiri commented on a diff in pull request #5195: HDFS-16856: Refactor RouterAdmin to use the AdminHelper class.
Posted by GitBox <gi...@apache.org>.
goiri commented on code in PR #5195:
URL: https://github.com/apache/hadoop/pull/5195#discussion_r1045147361
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -467,310 +293,265 @@ public int run(String[] argv) throws Exception {
*
* @throws IOException if the operation was not successful.
*/
- private int refreshSuperUserGroupsConfiguration()
+ private int refreshSuperUserGroupsConfiguration(Configuration conf,
+ List<String> args)
throws IOException{
- RouterGenericManager proxy = client.getRouterGenericManager();
- String address = getConf().getTrimmed(
- RBFConfigKeys.DFS_ROUTER_ADMIN_ADDRESS_KEY,
- RBFConfigKeys.DFS_ROUTER_ADMIN_ADDRESS_DEFAULT);
- if(proxy.refreshSuperUserGroupsConfiguration()){
- System.out.println(
- "Successfully updated superuser proxy groups on router " + address);
- return 0;
+ StringUtils.ensureAllUsed(args);
+ try (RouterClient client = createClient(conf)) {
+ RouterGenericManager proxy = client.getRouterGenericManager();
+ String address = getAddress(conf);
+ if (proxy.refreshSuperUserGroupsConfiguration()) {
+ System.out.println(
+ "Successfully updated superuser proxy groups on router " + address);
+ return 0;
+ }
}
return -1;
}
- private void refresh(String address) throws IOException {
- if (refreshRouterCache()) {
+ private int refresh(Configuration conf, List<String> args) throws IOException {
+ StringUtils.ensureAllUsed(args);
+ if (refreshRouterCache(conf)) {
System.out.println(
- "Successfully updated mount table cache on router " + address);
+ "Successfully updated mount table cache on router " + getAddress(conf));
+ return 0;
+ } else {
+ return -1;
}
}
/**
* Refresh mount table cache on connected router.
- *
+ * @param conf The configuration to use
* @return true if cache refreshed successfully
- * @throws IOException
*/
- private boolean refreshRouterCache() throws IOException {
- RefreshMountTableEntriesResponse response =
- client.getMountTableManager().refreshMountTableEntries(
- RefreshMountTableEntriesRequest.newInstance());
- return response.getResult();
+ private boolean refreshRouterCache(Configuration conf) throws IOException {
+ try (RouterClient client = createClient(conf)) {
+ RefreshMountTableEntriesResponse response =
+ client.getMountTableManager().refreshMountTableEntries(
+ RefreshMountTableEntriesRequest.newInstance());
+ return response.getResult();
+ }
}
-
/**
* Add a mount table entry or update if it exists.
- *
+ * @param conf Configuration
* @param parameters Parameters for the mount point.
- * @param i Index in the parameters.
- * @return If it was successful.
+ * @return 0 if it was successful.
* @throws IOException If it cannot add the mount point.
*/
- public boolean addMount(String[] parameters, int i) throws IOException {
- // Mandatory parameters
- String mount = parameters[i++];
- String[] nss = parameters[i++].split(",");
- String dest = parameters[i++];
-
- // Optional parameters
- boolean readOnly = false;
- boolean faultTolerant = false;
- String owner = null;
- String group = null;
- FsPermission mode = null;
- DestinationOrder order = DestinationOrder.HASH;
- while (i < parameters.length) {
- if (parameters[i].equals("-readonly")) {
- readOnly = true;
- } else if (parameters[i].equals("-faulttolerant")) {
- faultTolerant = true;
- } else if (parameters[i].equals("-order")) {
- i++;
- try {
- order = DestinationOrder.valueOf(parameters[i]);
- } catch(Exception e) {
- System.err.println("Cannot parse order: " + parameters[i]);
- }
- } else if (parameters[i].equals("-owner")) {
- i++;
- owner = parameters[i];
- } else if (parameters[i].equals("-group")) {
- i++;
- group = parameters[i];
- } else if (parameters[i].equals("-mode")) {
- i++;
- short modeValue = Short.parseShort(parameters[i], 8);
- mode = new FsPermission(modeValue);
- } else {
- printUsage("-add");
- return false;
+ public int addMount(Configuration conf,
+ List<String> parameters) throws IOException {
+ boolean readOnly = StringUtils.popOption("-readonly", parameters);
+ boolean faultTolerant = StringUtils.popOption("-faulttolerant", parameters);
+ String owner = StringUtils.popOptionWithArgument("-owner", parameters);
+ String group = StringUtils.popOptionWithArgument("-group", parameters);
+ String modeStr = StringUtils.popOptionWithArgument("-mode", parameters);
+ DestinationOrder order = DestinationOrder.valueOf(
+ StringUtils.popOptionWithArgument("-order", parameters, "HASH"));
+ String mount = StringUtils.popFirstNonOption(parameters);
+ String nssString = StringUtils.popFirstNonOption(parameters);
+ String dest = StringUtils.popFirstNonOption(parameters);
+ if (mount == null || nssString == null || dest == null) {
+ throw new IllegalArgumentException("Required parameters not supplied.");
+ }
+ StringUtils.ensureAllUsed(parameters);
+ String[] nss = nssString.split(",");
+
+ FsPermission mode = modeStr == null ?
+ null : new FsPermission(Short.parseShort(modeStr, 8));
+
+ return addMount(conf, mount, nss, dest, readOnly, faultTolerant, order,
+ new ACLEntity(owner, group, mode), true);
+ }
+
+ /**
+ * Check for an optional boolean argument.
+ * @param name the name of the argument
+ * @param parameters the list of all of the parameters
+ * @return null if the argument wasn't present, true or false based on the
+ * string
+ */
+ static Boolean parseOptionalBoolean(String name, List<String> parameters) {
+ String val = StringUtils.popOptionWithArgument(name, parameters);
+ if (val == null) {
+ return null;
+ } else {
+ switch (val.toLowerCase()) {
Review Comment:
Isn't there a parseBool or something?
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -467,310 +293,265 @@ public int run(String[] argv) throws Exception {
*
* @throws IOException if the operation was not successful.
*/
- private int refreshSuperUserGroupsConfiguration()
+ private int refreshSuperUserGroupsConfiguration(Configuration conf,
+ List<String> args)
throws IOException{
- RouterGenericManager proxy = client.getRouterGenericManager();
- String address = getConf().getTrimmed(
- RBFConfigKeys.DFS_ROUTER_ADMIN_ADDRESS_KEY,
- RBFConfigKeys.DFS_ROUTER_ADMIN_ADDRESS_DEFAULT);
- if(proxy.refreshSuperUserGroupsConfiguration()){
- System.out.println(
- "Successfully updated superuser proxy groups on router " + address);
- return 0;
+ StringUtils.ensureAllUsed(args);
+ try (RouterClient client = createClient(conf)) {
+ RouterGenericManager proxy = client.getRouterGenericManager();
+ String address = getAddress(conf);
+ if (proxy.refreshSuperUserGroupsConfiguration()) {
+ System.out.println(
+ "Successfully updated superuser proxy groups on router " + address);
+ return 0;
+ }
}
return -1;
}
- private void refresh(String address) throws IOException {
- if (refreshRouterCache()) {
+ private int refresh(Configuration conf, List<String> args) throws IOException {
+ StringUtils.ensureAllUsed(args);
+ if (refreshRouterCache(conf)) {
System.out.println(
- "Successfully updated mount table cache on router " + address);
+ "Successfully updated mount table cache on router " + getAddress(conf));
+ return 0;
+ } else {
+ return -1;
}
}
/**
* Refresh mount table cache on connected router.
- *
+ * @param conf The configuration to use
* @return true if cache refreshed successfully
- * @throws IOException
*/
- private boolean refreshRouterCache() throws IOException {
- RefreshMountTableEntriesResponse response =
- client.getMountTableManager().refreshMountTableEntries(
- RefreshMountTableEntriesRequest.newInstance());
- return response.getResult();
+ private boolean refreshRouterCache(Configuration conf) throws IOException {
+ try (RouterClient client = createClient(conf)) {
+ RefreshMountTableEntriesResponse response =
+ client.getMountTableManager().refreshMountTableEntries(
+ RefreshMountTableEntriesRequest.newInstance());
+ return response.getResult();
+ }
}
-
/**
* Add a mount table entry or update if it exists.
- *
+ * @param conf Configuration
* @param parameters Parameters for the mount point.
- * @param i Index in the parameters.
- * @return If it was successful.
+ * @return 0 if it was successful.
* @throws IOException If it cannot add the mount point.
*/
- public boolean addMount(String[] parameters, int i) throws IOException {
- // Mandatory parameters
- String mount = parameters[i++];
- String[] nss = parameters[i++].split(",");
- String dest = parameters[i++];
-
- // Optional parameters
- boolean readOnly = false;
- boolean faultTolerant = false;
- String owner = null;
- String group = null;
- FsPermission mode = null;
- DestinationOrder order = DestinationOrder.HASH;
- while (i < parameters.length) {
- if (parameters[i].equals("-readonly")) {
- readOnly = true;
- } else if (parameters[i].equals("-faulttolerant")) {
- faultTolerant = true;
- } else if (parameters[i].equals("-order")) {
- i++;
- try {
- order = DestinationOrder.valueOf(parameters[i]);
- } catch(Exception e) {
- System.err.println("Cannot parse order: " + parameters[i]);
- }
- } else if (parameters[i].equals("-owner")) {
- i++;
- owner = parameters[i];
- } else if (parameters[i].equals("-group")) {
- i++;
- group = parameters[i];
- } else if (parameters[i].equals("-mode")) {
- i++;
- short modeValue = Short.parseShort(parameters[i], 8);
- mode = new FsPermission(modeValue);
- } else {
- printUsage("-add");
- return false;
+ public int addMount(Configuration conf,
+ List<String> parameters) throws IOException {
+ boolean readOnly = StringUtils.popOption("-readonly", parameters);
+ boolean faultTolerant = StringUtils.popOption("-faulttolerant", parameters);
+ String owner = StringUtils.popOptionWithArgument("-owner", parameters);
+ String group = StringUtils.popOptionWithArgument("-group", parameters);
+ String modeStr = StringUtils.popOptionWithArgument("-mode", parameters);
+ DestinationOrder order = DestinationOrder.valueOf(
+ StringUtils.popOptionWithArgument("-order", parameters, "HASH"));
+ String mount = StringUtils.popFirstNonOption(parameters);
+ String nssString = StringUtils.popFirstNonOption(parameters);
+ String dest = StringUtils.popFirstNonOption(parameters);
+ if (mount == null || nssString == null || dest == null) {
+ throw new IllegalArgumentException("Required parameters not supplied.");
+ }
+ StringUtils.ensureAllUsed(parameters);
+ String[] nss = nssString.split(",");
+
+ FsPermission mode = modeStr == null ?
+ null : new FsPermission(Short.parseShort(modeStr, 8));
+
+ return addMount(conf, mount, nss, dest, readOnly, faultTolerant, order,
+ new ACLEntity(owner, group, mode), true);
+ }
+
+ /**
+ * Check for an optional boolean argument.
+ * @param name the name of the argument
+ * @param parameters the list of all of the parameters
+ * @return null if the argument wasn't present, true or false based on the
+ * string
+ */
+ static Boolean parseOptionalBoolean(String name, List<String> parameters) {
+ String val = StringUtils.popOptionWithArgument(name, parameters);
+ if (val == null) {
+ return null;
+ } else {
Review Comment:
As you return before, skip the else.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java:
##########
@@ -1182,6 +1200,19 @@ public static String popFirstNonOption(List<String> args) {
}
return null;
}
+ /**
+ * From a list of command-line arguments, ensure that all of the arguments
+ * have been used except a possible "--".
+ *
+ * @param args List of arguments.
+ * @throws IllegalArgumentException if some arguments were not used
+ */
+ public static void ensureAllUsed(List<String> args) throws IllegalArgumentException {
+ if (!args.isEmpty() && !(args.size() == 1 && "--".equals(args.get(0)))) {
Review Comment:
I don't think we need some of the parenthesis.
##########
hadoop-hdfs-project/hadoop-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/tools/federation/RouterAdmin.java:
##########
@@ -439,24 +266,23 @@ public int run(String[] argv) throws Exception {
try {
String[] content;
content = e.getLocalizedMessage().split("\n");
- System.err.println(cmd.substring(1) + ": " + content[0]);
+ System.err.println(cmd+ ": " + content[0]);
Review Comment:
Space.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] omalley commented on a diff in pull request #5195: HDFS-16856: Refactor RouterAdmin to use the AdminHelper class.
Posted by GitBox <gi...@apache.org>.
omalley commented on code in PR #5195:
URL: https://github.com/apache/hadoop/pull/5195#discussion_r1046122861
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/StringUtils.java:
##########
@@ -1182,6 +1200,19 @@ public static String popFirstNonOption(List<String> args) {
}
return null;
}
+ /**
+ * From a list of command-line arguments, ensure that all of the arguments
+ * have been used except a possible "--".
+ *
+ * @param args List of arguments.
+ * @throws IllegalArgumentException if some arguments were not used
+ */
+ public static void ensureAllUsed(List<String> args) throws IllegalArgumentException {
+ if (!args.isEmpty() && !(args.size() == 1 && "--".equals(args.get(0)))) {
Review Comment:
I find writing code that depends overly on knowing the precedence for non-math operators leads to trouble, so I'd prefer to leave them in.
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org