You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/06/03 07:30:10 UTC
[GitHub] [ozone] sadanand48 opened a new pull request #2303: HDDS-5283.getStorageSize cast to int can cause issue
sadanand48 opened a new pull request #2303:
URL: https://github.com/apache/ozone/pull/2303
## What changes were proposed in this pull request?
value of `pendingRequestsByteLimit` couldn’t be set beyond 2GB because of the integer limit. Similarly there were other places where getStorageSize was cast to int. Converted to long where possible and changed the scale of the variable if not.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5283
## How was this patch tested?
CI
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#discussion_r689243109
##########
File path: pom.xml
##########
@@ -72,7 +72,7 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
<declared.ozone.version>${ozone.version}</declared.ozone.version>
<!-- Apache Ratis version -->
- <ratis.version>2.1.0</ratis.version>
+ <ratis.version>2.1.0-03f3b68-SNAPSHOT</ratis.version>
Review comment:
Sorry, but this seems to be wrong. Ratis 2.1.0 is already released, we cannot have a new snapshot version for it. I think we should go for a new 2.1.1 release with RATIS-1384 (and RATIS-1386) bugfix.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mukul1987 commented on a change in pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#discussion_r682762322
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
##########
@@ -612,4 +615,11 @@ public static String format(List<String> nodes) {
public static long getShutDownTimeOut(ConfigurationSource conf) {
return conf.getObject(OzoneServiceConfig.class).getServiceShutdownTimeout();
}
+
+ /**
+ * Utility method to round up bytes into the nearest MB.
+ */
+ public static int roundupMb(long bytes) {
+ return Math.toIntExact((bytes - 1) / ONE_MB + 1);
Review comment:
This will always compute to a long, and this will not be a floating point number. Also I think there should be a utility to perform 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mukul1987 commented on a change in pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
mukul1987 commented on a change in pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#discussion_r682762322
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java
##########
@@ -612,4 +615,11 @@ public static String format(List<String> nodes) {
public static long getShutDownTimeOut(ConfigurationSource conf) {
return conf.getObject(OzoneServiceConfig.class).getServiceShutdownTimeout();
}
+
+ /**
+ * Utility method to round up bytes into the nearest MB.
+ */
+ public static int roundupMb(long bytes) {
+ return Math.toIntExact((bytes - 1) / ONE_MB + 1);
Review comment:
This will always compute to a long, and this will not be a floating point number. Also I think there should be a utility to perform 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#discussion_r649878637
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -164,13 +164,14 @@ public ContainerStateMachine(RaftGroupId gid, ContainerDispatcher dispatcher,
int numPendingRequests = conf
.getObject(DatanodeRatisServerConfig.class)
.getLeaderNumPendingRequests();
- int pendingRequestsByteLimit = (int) conf.getStorageSize(
+ int pendingRequestsMegaBytesLimit = (int) conf.getStorageSize(
OzoneConfigKeys.DFS_CONTAINER_RATIS_LEADER_PENDING_BYTES_LIMIT,
OzoneConfigKeys.DFS_CONTAINER_RATIS_LEADER_PENDING_BYTES_LIMIT_DEFAULT,
- StorageUnit.BYTES);
+ StorageUnit.MB);
stateMachineDataCache = new ResourceLimitCache<>(new ConcurrentHashMap<>(),
- (index, data) -> new int[] {1, data.size()}, numPendingRequests,
- pendingRequestsByteLimit);
+ (index, data) -> new int[] {1,
+ (int) StorageUnit.MB.fromBytes(data.size()) }, numPendingRequests,
Review comment:
Question if data.size() is in bytes, this conversion will yield to zero? And what locks we acquire for the bytes limit?
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
##########
@@ -437,12 +438,13 @@ private RpcType setRpcType(RaftProperties properties) {
private void setPendingRequestsLimits(RaftProperties properties) {
- final int pendingRequestsByteLimit = (int)conf.getStorageSize(
+ final int pendingRequestsMegaBytesLimit = (int)conf.getStorageSize(
OzoneConfigKeys.DFS_CONTAINER_RATIS_LEADER_PENDING_BYTES_LIMIT,
OzoneConfigKeys.DFS_CONTAINER_RATIS_LEADER_PENDING_BYTES_LIMIT_DEFAULT,
- StorageUnit.BYTES);
+ StorageUnit.MB);
Review comment:
Does this need Ratis Change also, like how you are converting Data size to MB which you are doing in StateMachine?
because in ratis
```
tryAcquire(1, Message.getSize(message)
```
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bshashikant commented on pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
bshashikant commented on pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#issuecomment-884995416
@sadanand48 , can you please rebase?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] mukul1987 merged pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
mukul1987 merged pull request #2303:
URL: https://github.com/apache/ozone/pull/2303
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#discussion_r689243109
##########
File path: pom.xml
##########
@@ -72,7 +72,7 @@ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xs
<declared.ozone.version>${ozone.version}</declared.ozone.version>
<!-- Apache Ratis version -->
- <ratis.version>2.1.0</ratis.version>
+ <ratis.version>2.1.0-03f3b68-SNAPSHOT</ratis.version>
Review comment:
Sorry, but this seems to be wrong. Ratis 2.1.0 is already released, we cannot have a new snapshot version for it. I think we should go for a new 2.1.1 release with RATIS-1384 (and RATIS-1386).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ferhui commented on pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
ferhui commented on pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#issuecomment-912486762
@mukul1987 @adoroszlai Hi compile fails because of @adoroszlai 's last comments. Should we change the ratis version?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2303: HDDS-5283. getStorageSize cast to int can cause issue
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2303:
URL: https://github.com/apache/ozone/pull/2303#discussion_r645438265
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java
##########
@@ -164,13 +164,13 @@ public ContainerStateMachine(RaftGroupId gid, ContainerDispatcher dispatcher,
int numPendingRequests = conf
.getObject(DatanodeRatisServerConfig.class)
.getLeaderNumPendingRequests();
- int pendingRequestsByteLimit = (int) conf.getStorageSize(
+ int pendingRequestsMegaBytesLimit = (int) conf.getStorageSize(
OzoneConfigKeys.DFS_CONTAINER_RATIS_LEADER_PENDING_BYTES_LIMIT,
OzoneConfigKeys.DFS_CONTAINER_RATIS_LEADER_PENDING_BYTES_LIMIT_DEFAULT,
- StorageUnit.BYTES);
+ StorageUnit.MB);
stateMachineDataCache = new ResourceLimitCache<>(new ConcurrentHashMap<>(),
(index, data) -> new int[] {1, data.size()}, numPendingRequests,
Review comment:
@sadanand48 I think acceptance test failure is caused by this change. 1KB keys can be created, but anything beyond that fails.
Limit is in megabytes, `data.size()` needs to be converted.
```suggestion
(i, data) -> new int[] {1, (int) StorageUnit.MB.fromBytes(data.size())},
numPendingRequests,
```
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org