keith-turner opened a new pull request, #5550:
URL: https://github.com/apache/accumulo/pull/5550
Thrift supports the following two configurations related to incoming framed
messages sizes.
1. A max size for individual message. When a message is over this size
thrift will close the connection and not read the message. Accumulo configure
that setting
[here](https://github.com/apache/accumulo/blob/1e4b7924a13f1fe1883c7de3c8faca7fdb4791ce/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java#L273)
2. A max size for all messages across all connections that will be held in
memory. If a message on a connection would exceed this limit, then thrift
delays reading the data off the connection until there is enough memory.
Accumulo configures that setting
[here](https://github.com/apache/accumulo/blob/1e4b7924a13f1fe1883c7de3c8faca7fdb4791ce/server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java#L280)
The thrift code implementing to these two configurations can be seen
[here](https://github.com/apache/thrift/blob/df626d768a87fe07fef215b4dde831185e6929d7/lib/java/src/main/java/org/apache/thrift/server/AbstractNonblockingServer.java#L342-L356).
Accumulo configures both thrift settings to the same value. Experimented
with adding a seperate setting for the total across all configurations in this
PR. Also added a ton of logging and a test to convince myself this is actually
working, and it is working will see thrift deferring reading from a connection
when there is too much already in memory. Can not think of a way to actaully
test this feature because it will eventually work, there is just an
indeterminate delay.
One uncertainty I have about adding this new property is the way it changes
behavior. The existing accumulo property used to configure two thrift settings
in code. After this change the existing accumulo property only configures one
thrift setting code and the new accumulo property configures the other thrift
setting. This changes seem tricky for the case where the existing property is
already set. That uncertainty about the new property and not having an ability
to test this new property are the reasons why I am opening this as draft for
now. The test that was added was used to cause activity that would allow me
to look in the logs and verify this is working, it does not actually test the
new property is working.
--
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]
