[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r322930998
##
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##
@@ -131,6 +142,13 @@ public void initialize(URI name, Configuration conf)
throws IOException {
// If port number is not specified, read it from config
omPort = OmUtils.getOmRpcPort(conf);
}
+} else if (OmUtils.isServiceIdsDefined(conf)) {
Review comment:
https://github.com/apache/hadoop/blob/trunk/hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java#L154
This is code line calling creation of BasicOzoneClientAdapterImpl. And below
is the code where it checks if conf passed is instance of OzoneConfiguration or
not, if not convert to OzoneConfiguration object.
https://github.com/apache/hadoop/blob/trunk/hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java#L112
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids URL: https://github.com/apache/hadoop/pull/1360#discussion_r321926066 ## File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java ## @@ -214,6 +216,11 @@ public OzoneManagerProtocolClientSideTranslatorPB(OzoneConfiguration conf, this.clientID = clientId; } + public OzoneManagerProtocolClientSideTranslatorPB(OzoneConfiguration conf, Review comment: Yes if possible. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321925879
##
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##
@@ -137,12 +137,22 @@
private Text dtService;
private final boolean topologyAwareReadEnabled;
+ /**
+ * Creates RpcClient instance with the given configuration.
+ * @param conf Configuration
+ * @throws IOException
+ */
+ public RpcClient(Configuration conf) throws IOException {
Review comment:
My comment is for add to a notion of @VisibleForTesting, and I think this
can also be removed, as it is used only for testing. And it can be completely
removed, and use the new constructor.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids URL: https://github.com/apache/hadoop/pull/1360#discussion_r321587112 ## File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java ## @@ -214,6 +216,11 @@ public OzoneManagerProtocolClientSideTranslatorPB(OzoneConfiguration conf, this.clientID = clientId; } + public OzoneManagerProtocolClientSideTranslatorPB(OzoneConfiguration conf, Review comment: This can be removed, no one is calling this. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321586979
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##
@@ -70,26 +71,46 @@
private final UserGroupInformation ugi;
private final Text delegationTokenService;
+ // TODO: Do we want this to be final?
+ private String omServiceId;
+
public OMFailoverProxyProvider(OzoneConfiguration configuration,
- UserGroupInformation ugi) throws IOException {
+ UserGroupInformation ugi, String omServiceId) throws IOException {
this.conf = configuration;
this.omVersion = RPC.getProtocolVersion(OzoneManagerProtocolPB.class);
this.ugi = ugi;
-loadOMClientConfigs(conf);
+this.omServiceId = omServiceId;
+loadOMClientConfigs(conf, this.omServiceId);
this.delegationTokenService = computeDelegationTokenService();
currentProxyIndex = 0;
currentProxyOMNodeId = omNodeIDList.get(currentProxyIndex);
}
- private void loadOMClientConfigs(Configuration config) throws IOException {
+ public OMFailoverProxyProvider(OzoneConfiguration configuration,
+ UserGroupInformation ugi) throws IOException {
+this(configuration, ugi, null);
+ }
+
+ private void loadOMClientConfigs(Configuration config, String omSvcId)
+ throws IOException {
this.omProxies = new HashMap<>();
this.omProxyInfos = new HashMap<>();
this.omNodeIDList = new ArrayList<>();
-Collection omServiceIds = config.getTrimmedStringCollection(
-OZONE_OM_SERVICE_IDS_KEY);
+Collection omServiceIds;
+if (omSvcId == null) {
Review comment:
Thinking more, we can refactor this below first loop, we don't need at all.
You can take up this refactor in a new jira also.
for (String serviceId : OmUtils.emptyAsSingletonNull(omServiceIds)) {
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321586979
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##
@@ -70,26 +71,46 @@
private final UserGroupInformation ugi;
private final Text delegationTokenService;
+ // TODO: Do we want this to be final?
+ private String omServiceId;
+
public OMFailoverProxyProvider(OzoneConfiguration configuration,
- UserGroupInformation ugi) throws IOException {
+ UserGroupInformation ugi, String omServiceId) throws IOException {
this.conf = configuration;
this.omVersion = RPC.getProtocolVersion(OzoneManagerProtocolPB.class);
this.ugi = ugi;
-loadOMClientConfigs(conf);
+this.omServiceId = omServiceId;
+loadOMClientConfigs(conf, this.omServiceId);
this.delegationTokenService = computeDelegationTokenService();
currentProxyIndex = 0;
currentProxyOMNodeId = omNodeIDList.get(currentProxyIndex);
}
- private void loadOMClientConfigs(Configuration config) throws IOException {
+ public OMFailoverProxyProvider(OzoneConfiguration configuration,
+ UserGroupInformation ugi) throws IOException {
+this(configuration, ugi, null);
+ }
+
+ private void loadOMClientConfigs(Configuration config, String omSvcId)
+ throws IOException {
this.omProxies = new HashMap<>();
this.omProxyInfos = new HashMap<>();
this.omNodeIDList = new ArrayList<>();
-Collection omServiceIds = config.getTrimmedStringCollection(
-OZONE_OM_SERVICE_IDS_KEY);
+Collection omServiceIds;
+if (omSvcId == null) {
Review comment:
Thinking more, we can refactor this below first loop, we don't need at all.
You can take up this refactor in a new jira also.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321586721
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##
@@ -70,26 +71,46 @@
private final UserGroupInformation ugi;
private final Text delegationTokenService;
+ // TODO: Do we want this to be final?
+ private String omServiceId;
+
public OMFailoverProxyProvider(OzoneConfiguration configuration,
- UserGroupInformation ugi) throws IOException {
+ UserGroupInformation ugi, String omServiceId) throws IOException {
this.conf = configuration;
this.omVersion = RPC.getProtocolVersion(OzoneManagerProtocolPB.class);
this.ugi = ugi;
-loadOMClientConfigs(conf);
+this.omServiceId = omServiceId;
+loadOMClientConfigs(conf, this.omServiceId);
this.delegationTokenService = computeDelegationTokenService();
currentProxyIndex = 0;
currentProxyOMNodeId = omNodeIDList.get(currentProxyIndex);
}
- private void loadOMClientConfigs(Configuration config) throws IOException {
+ public OMFailoverProxyProvider(OzoneConfiguration configuration,
+ UserGroupInformation ugi) throws IOException {
+this(configuration, ugi, null);
+ }
+
+ private void loadOMClientConfigs(Configuration config, String omSvcId)
+ throws IOException {
this.omProxies = new HashMap<>();
this.omProxyInfos = new HashMap<>();
this.omNodeIDList = new ArrayList<>();
-Collection omServiceIds = config.getTrimmedStringCollection(
-OZONE_OM_SERVICE_IDS_KEY);
+Collection omServiceIds;
+if (omSvcId == null) {
+ // When no OM service id is passed in
+ // Note: this branch will only be followed when omSvcId is null,
+ // meaning the host name/service id provided by user doesn't match any
+ // ozone.om.service.ids on the client side. Therefore, in this case
+ // just treat it as non-HA by assigning an empty list to omServiceIds
+ omServiceIds = new ArrayList<>();
+} else {
+ omServiceIds = Collections.singletonList(omSvcId);
+}
+// TODO: Remove this warning? Or change the message?
Review comment:
We don't need this condition at all right, as this condition will never
occur. As now serviceId is passed.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321586333
##
File path:
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/ha/OMFailoverProxyProvider.java
##
@@ -70,26 +71,46 @@
private final UserGroupInformation ugi;
private final Text delegationTokenService;
+ // TODO: Do we want this to be final?
+ private String omServiceId;
+
public OMFailoverProxyProvider(OzoneConfiguration configuration,
- UserGroupInformation ugi) throws IOException {
+ UserGroupInformation ugi, String omServiceId) throws IOException {
this.conf = configuration;
this.omVersion = RPC.getProtocolVersion(OzoneManagerProtocolPB.class);
this.ugi = ugi;
-loadOMClientConfigs(conf);
+this.omServiceId = omServiceId;
+loadOMClientConfigs(conf, this.omServiceId);
this.delegationTokenService = computeDelegationTokenService();
currentProxyIndex = 0;
currentProxyOMNodeId = omNodeIDList.get(currentProxyIndex);
}
- private void loadOMClientConfigs(Configuration config) throws IOException {
+ public OMFailoverProxyProvider(OzoneConfiguration configuration,
+ UserGroupInformation ugi) throws IOException {
+this(configuration, ugi, null);
+ }
+
+ private void loadOMClientConfigs(Configuration config, String omSvcId)
+ throws IOException {
this.omProxies = new HashMap<>();
this.omProxyInfos = new HashMap<>();
this.omNodeIDList = new ArrayList<>();
-Collection omServiceIds = config.getTrimmedStringCollection(
-OZONE_OM_SERVICE_IDS_KEY);
+Collection omServiceIds;
+if (omSvcId == null) {
Review comment:
```
if (omSvcId == null) {
// When no OM service id is passed in
// Note: this branch will only be followed when omSvcId is null,
// meaning the host name/service id provided by user doesn't match any
// ozone.om.service.ids on the client side. Therefore, in this case
// just treat it as non-HA by assigning an empty list to omServiceIds
omServiceIds = new ArrayList<>();
} else {
omServiceIds = Collections.singletonList(omSvcId);
}
```
Can be replaced with `omServiceIds = Collections.singletonList(omSvcId)`;
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321585352
##
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##
@@ -137,12 +137,22 @@
private Text dtService;
private final boolean topologyAwareReadEnabled;
+ /**
+ * Creates RpcClient instance with the given configuration.
+ * @param conf Configuration
+ * @throws IOException
+ */
+ public RpcClient(Configuration conf) throws IOException {
Review comment:
Can we mark this @VisibleForTesting or we can remove this method and use
other overloaded constructor method.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321585352
##
File path:
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##
@@ -137,12 +137,22 @@
private Text dtService;
private final boolean topologyAwareReadEnabled;
+ /**
+ * Creates RpcClient instance with the given configuration.
+ * @param conf Configuration
+ * @throws IOException
+ */
+ public RpcClient(Configuration conf) throws IOException {
Review comment:
Can we mark this @VisibleForTesting
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids URL: https://github.com/apache/hadoop/pull/1360#discussion_r321585214 ## File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneClientFactory.java ## @@ -136,6 +136,31 @@ public static OzoneClient getRpcClient(String omHost, Integer omRpcPort, return getRpcClient(config); } + /** + * Returns an OzoneClient which will use RPC protocol. + * + * @param omServiceId + *Service ID of OzoneManager HA cluster. + * + * @param config + *Configuration to be used for OzoneClient creation + * + * @return OzoneClient + * + * @throws IOException + */ + public static OzoneClient getRpcClient(String omServiceId, Review comment: One suggestion, can we have a single getRpcClient method, instead of multiple methods. Because in all cases, I think we can call this method if the configuration is set with om address in non-HA case. 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321583946
##
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##
@@ -131,6 +142,13 @@ public void initialize(URI name, Configuration conf)
throws IOException {
// If port number is not specified, read it from config
omPort = OmUtils.getOmRpcPort(conf);
}
+} else if (OmUtils.isServiceIdsDefined(conf)) {
+ // When host name or service id is given, and ozone.om.service.ids is
Review comment:
We arrived at this point means, hostname or service id is not provided in
url 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make ozone fs shell command work with OM HA service ids
bharatviswa504 commented on a change in pull request #1360: HDDS-2007. Make
ozone fs shell command work with OM HA service ids
URL: https://github.com/apache/hadoop/pull/1360#discussion_r321583418
##
File path:
hadoop-ozone/ozonefs/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneFileSystem.java
##
@@ -131,6 +142,13 @@ public void initialize(URI name, Configuration conf)
throws IOException {
// If port number is not specified, read it from config
omPort = OmUtils.getOmRpcPort(conf);
}
+} else if (OmUtils.isServiceIdsDefined(conf)) {
Review comment:
Here conf passed might not be ozone configuration object, to get config
values of ozone-site.xml, we need to convert conf to ozone configuration object.
`OmUtils.isServiceIdsDefined(new OzoneConfiguration(conf))`
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
