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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
