New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rally PMC performance regression due to compression #36399
Comments
Pinging @elastic/es-distributed |
I think that we should only compress if |
Yes to clarify, the "setting" means whichever compression setting applies based on the rules introduced in #35357 and outlined in #34483. If @s1monw agrees, I can remove compression from the |
Confirmed, the network connection between the benchmark machines is a dedicated 10Gbps link. |
+1 |
Currently TransportRequestOptions allows specific requests to request compression. This commit removes this and always compresses based on the settings. Additionally, it removes TransportResponseOptions as they are unused. This closes #36399.
Currently TransportRequestOptions allows specific requests to request compression. This commit removes this and always compresses based on the settings. Additionally, it removes TransportResponseOptions as they are unused. This closes elastic#36399.
Currently TransportRequestOptions allows specific requests to request compression. This commit removes this and always compresses based on the settings. Additionally, it removes TransportResponseOptions as they are unused. This closes #36399.
After #36522 was merged, our PMC benchmarks reverted to their prior performance level. |
This is a follow-up to some discussions around elastic#36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on `transport.compress` or a specific setting for a remote cluster. However, we can only compress responses based on `transport.compress` as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.
This is a follow-up to some discussions around #36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on transport.compress or a specific setting for a remote cluster. However, we can only compress responses based on transport.compress as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.
This is a follow-up to some discussions around elastic#36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on transport.compress or a specific setting for a remote cluster. However, we can only compress responses based on transport.compress as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.
This is a follow-up to some discussions around #36399. Currently we have relatively confusing compression behavior where compression can be configured for requests based on transport.compress or a specific setting for a remote cluster. However, we can only compress responses based on transport.compress as we do not know where a request is coming from (currently). This commit modifies the behavior to NEVER compress responses based on settings. Instead, a response will only be compressed if the request was compressed. This commit also updates the documentation to more clearly described transport level compression.
Around early November we experienced a noticeable performance regression on the PMC track in our nightly benchmarks. This only applied to multi-node (network) workloads and was apparent on both the netty and nio runs.
This is very visible on the 90-day graph.
@dliappis identified that it was due to #35357. I investigated this and identified what the root cause is.
Individual actions in Elasticsearch can set compression to
true
inTransportRequestOptions
.A number of actions set this option manually:
TransportGetTaskAction
-false
BulkAction
-true
TransportNodesAction.AsyncAction
- dependsTransportTasksAction
- dependsPublishClusterStateAction
-false
RemoteRecoveryTargetHandler
- dependsPrior to #35357 this option had no effect. If it was set to
false
,TcpTransport
would overwrite it iftransport.tcp.compress
was set totrue
.If it was set to
true
,TcpTransport
would only respect it iftransport.tcp.compress
was also set to true.After #35357, we still overwrite
false
if compression is enabled based on settings. However, we incidentally started to respecttrue
. So if compression was not enabled we would still compress if it was requested by theTransportRequestOptions
.This one change led to the performance regression because the PMC benchmark probably uses messages that ask for compression. Our benchmarks set the compression setting to false. So these messages were not compressed before the change and are compressed after the change. Since the benchmarks (I think) have fast network connections, this compression likely increases CPU usage and the bandwidth savings are probably irrelevant.
This raises the question of what we want the behavior to be. The behavior prior to #35357 makes no sense as there was no point in the compression being set in the
TransportRequestOptions
.Do we want:
true
?true
? This would overwrite false. This is equivalent to post-Move compression config to ConnectionProfile #35357 behavior.TransportRequestOptions
be the primary choice and the setting be the fallback choice? This is tricky right now becauseTransportRequestOptions
uses aboolean
for the compression indicator. So we do not know iffalse
was set or if it was not set.TransportRequestOptions
and always compress based on the setting. This is equivalent to pre-Move compression config to ConnectionProfile #35357 behavior.@jasontedor @s1monw
The text was updated successfully, but these errors were encountered: