[
https://issues.apache.org/jira/browse/HDFS-16856?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17645696#comment-17645696
]
ASF GitHub Bot commented on HDFS-16856:
---------------------------------------
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.
> RBF: Refactor router admin command to use HDFS AdminHelper class
> ----------------------------------------------------------------
>
> Key: HDFS-16856
> URL: https://issues.apache.org/jira/browse/HDFS-16856
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: rbf
> Reporter: Owen O'Malley
> Assignee: Owen O'Malley
> Priority: Major
> Labels: pull-request-available
>
> Currently, the router admin class is a bit of a mess with a lot of custom
> programming. We should use the infrastructure that was developed in the
> AdminHelper class to standardize the command processing.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
