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 2022/02/23 23:49:53 UTC
[GitHub] [ozone] hanishakoneru commented on a change in pull request #3024: HDDS-6162. Limit OM pending request size to avoid taking too much memory
hanishakoneru commented on a change in pull request #3024:
URL: https://github.com/apache/ozone/pull/3024#discussion_r813413774
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -292,4 +292,8 @@ private OMConfigKeys() {
public static final long OZONE_OM_ADMIN_PROTOCOL_WAIT_BETWEEN_RETRIES_DEFAULT
= 1000;
+ public static final String OZONE_OM_MAX_PENDING_REQ_COUNT =
+ "ozone.om.max.pending.req.count";
+ public static final int OZONE_OM_MAX_PENDING_REQ_COUNT_DEFAULT = 10000;
Review comment:
How was this default value decided? Is there any benchmark we can take a look at to see how the performance is affected by this setting?
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/OMConfigKeys.java
##########
@@ -292,4 +292,8 @@ private OMConfigKeys() {
public static final long OZONE_OM_ADMIN_PROTOCOL_WAIT_BETWEEN_RETRIES_DEFAULT
= 1000;
+ public static final String OZONE_OM_MAX_PENDING_REQ_COUNT =
+ "ozone.om.max.pending.req.count";
Review comment:
ozone.om.max.pending.req.count gives me the idea that it is max pending queue size client requests to OM. Can we rename this to something like ozone.om.double.buffer.max.size or ozone.om.unflushed.transactions.max.size or something to denote it is for the OM DoubleBuffer.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java
##########
@@ -147,22 +150,29 @@ public Builder setIndexToTerm(Function<Long, Long> termGet) {
return this;
}
+ public Builder setAvailPendingRequestNum(Semaphore pendingReqNum) {
Review comment:
It's good to keep the builder as lightweight as possible. Instead of passing the Semaphore, can we pass the maxRequestsInDoubleBuffer as an int parameter and initialize the Semaphore in OzoneManagerDoubleBuffer's constructor.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerStateMachine.java
##########
@@ -308,6 +316,10 @@ public TransactionContext preAppendTransaction(TransactionContext trx)
CompletableFuture<Message> ratisFuture =
new CompletableFuture<>();
applyTransactionMap.put(trxLogIndex, trx.getLogEntry().getTerm());
+
+ //if there are too many pending requests, wait for doubleBuffer flushing
+ availPendingRequestNum.acquire();
+
Review comment:
It's good practice to keep the Semaphores private and have acquire and release methods for them. It helps in keeping track of where these objects are used.
Instead of initializing the Semaphore in OzoneManagerStateMachine and passing it over to OzoneManagerDoubleBuffer, can we keep the Semaphores private in OzoneManagerDoubleBuffer and access them via public methods.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/OzoneManagerDoubleBuffer.java
##########
@@ -120,6 +122,7 @@
private boolean isRatisEnabled = false;
private boolean isTracingEnabled = false;
private Function<Long, Long> indexToTerm = null;
+ private Semaphore availPendingRequestNum;
Review comment:
This variable name is little confusing. Pending request seems like the request still needs to be processed. We can say unflushed transactions or something.
--
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