ctubbsii commented on code in PR #2632:
URL: https://github.com/apache/accumulo/pull/2632#discussion_r852278160
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -506,6 +506,10 @@ public enum Property {
+ " tserver.wal.max.size. Ensure that
table.compaction.minor.logs.threshold"
+ " * tserver.wal.max.size >= this property.",
"1.3.5"),
+ TSERV_WRITE_THREADS_MAX("tserver.write.threads.max", "0", PropertyType.COUNT,
+ "The maximum number of write threads allowed for tablet servers."
+ + "When set to 0, there is no limit.",
Review Comment:
Could clarify `non-metadata write threads`.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ThriftClientHandler.java:
##########
@@ -686,16 +685,27 @@ private void setUpdateTablet(UpdateSession us, KeyExtent
keyExtent) {
@Override
public void applyUpdates(TInfo tinfo, long updateID, TKeyExtent tkeyExtent,
- List<TMutation> tmutations) {
+ List<TMutation> tmutations) throws TException {
UpdateSession us = (UpdateSession)
server.sessionManager.reserveSession(updateID);
if (us == null) {
return;
}
+ ThreadPoolExecutor writeThreadExecutor = server.getWriteThreadExecutor();
boolean reserved = true;
+
try {
KeyExtent keyExtent = KeyExtent.fromThrift(tkeyExtent);
- setUpdateTablet(us, keyExtent);
+
+ // get write thread pool handler but only for user tablets
+ if (TabletType.type(keyExtent) == TabletType.USER) {
+ if (writeThreadExecutor.isTerminating()) {
+ throw new TException("Write Thread pool is terminating");
+ }
+ writeThreadExecutor.execute(() -> setUpdateTablet(us, keyExtent));
Review Comment:
I'm not sure there's a benefit to doing setUpdateTablet in a separate
thread. I don't think that's where the action is happening. I think the bulk of
the action is happening further down in this method.
Before any of this code is changed, I think one could argue that each of
these threads that is handling the Thrift request are "write threads". We can't
really limit the number of threads that are created, because that's a function
of the RPC handling. However, I think the idea was to limit the number of these
threads that are doing meaningful work by:
1. determine if the thread is a user thread, and if so, then,
2. perform the entire rest of the work to handle this RPC request in a
separate size-limited executor service, while this RPC thread waits for it to
complete before returning to the client.
So, if I understand this code correctly, it wouldn't just be calling
setUpdateTablet in a separate thread... but doing all the work that this method
is doing in a separate thread.
Honestly, though, I'm not sure what the initial problem was that led to this
feature request. I don't know what the bottleneck was for having all of these
current RPC threads behaving as "write threads" without a limit.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -410,6 +420,18 @@ void requestStop() {
serverStopRequested = true;
}
+ public ThreadPoolExecutor getWriteThreadExecutor() {
+ var configured =
+
getServerConfig().getConfiguration().getCount(Property.TSERV_WRITE_THREADS_MAX);
+ // resize pool if config changed
+ if (INITIAL_MAX_WRITE_THREADS != configured) {
+ if (configured == 0) {
+ return writeThreadExecutor;
+ }
+ }
+ return writeThreadExecutor;
Review Comment:
This doesn't look like it's doing any resizing at all. Is this a follow-on
action? I think we could use the deriver stuff in the configuration to detect
when resize is needed, rather than trying to save the initial configuration and
watch it manually. I know we have another thread pool that is dynamically
resized. This could be done however that one is being done.
##########
core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java:
##########
@@ -330,6 +330,23 @@ public int getCount(Property property) {
return Integer.parseInt(countString);
}
+ /**
+ * Gets the default value of a property of type {@link PropertyType#COUNT},
interpreting the value
+ * properly (as an integer).
+ *
+ * @param property
+ * property to get default value
+ * @return property default value
+ * @throws IllegalArgumentException
+ * if the property is of the wrong type
+ */
+ public int getDefaultCount(Property property) {
Review Comment:
It should be removed. It is equivalent to, and redundant with,
`DefaultConfiguration.getInstance().getCount(property)`.
--
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]
