Skip to content
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

Fixes updates to traffic shaping options with shared servers #5282

Open
wants to merge 2 commits into
base: 4.x
Choose a base branch
from

Conversation

arjunashok
Copy link

Motivation:

This fixes the update-path for traffic shaping options to check for the existence of the traffic shaping handler only for the actual server. Currently, updating traffic-shaping options in a shared-server setting results in IllegalStateException, as the traffic-shaping handler is not set for the worker (non-main) servers.

Conformance:

You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines

This fixes the update-path for traffic shaping options to check for the existence of the traffic shaping handler only for the actual server.
@arjunashok arjunashok marked this pull request as draft August 14, 2024 22:18
@arjunashok arjunashok marked this pull request as ready for review August 14, 2024 22:19
if (server != null && server != this) {
server.updateTrafficShapingOptions(options);
server.updateTrafficShapingOptions(options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this, with return here to avoid repeated calls

Copy link
Author

@arjunashok arjunashok Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interesting question. The repeated calls would happen if the consumer of this API invokes this method for all the shared servers.

The decision here is based on the contract/interface we would like to expose to clients invoking updateTrafficShapingOptions (and similar APIs that affect configuration across shared servers)

  1. Is it fair to expect clients to apply/change this setting for all the servers, while having the underlying implementation only update it when the actual server is involved? (which is what I think you are suggesting)

  2. Alternatively, should we define the contract such that this setting should be applied/changed to ANY of the servers, and under the hood, vertx will only update it for the actual/main server? (current implementation)

It seems like we currently have a model like (1). My only concern is that for invocations that are not on a actual/main server, it will do nothing (silently).
If this is indeed the pattern we would like to follow, then I would prefer to have some signal (even if it were a log line) indicating that this was a worker server, so no updates were made. This might be useful, in the event that someone updates a server’s setting (that is not a main server) to not see the change taking effect.

I see that updateSSLOptions does something similar to (1), although if it were called for all the servers, we would update the SSL options for the actual server N times (where N = no. verticles)

(2) comes with the assumption that this is a shared/global setting that applies to all servers, and hence can be updated on ANY of the servers. Although, one could argue that it is ambiguous for the client as to which server's configuration should be updated, and if they were to apply this to all servers, it would result in several redundant invocations for the actual server.

@vietj Do you have any thoughts or comments on this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit, I made a change to only update the traffic-shaping options when they change. So, we address both concerns by:

  1. Updating the actual-server's options regardless of which server is used for the update
  2. Only perform the above update when the new options differ from the existing ones

…hey do not change

This is especially relevant for shared servers, where we now 1) update the actual-server's options, and 2) only perform the update when the traffic options change.
@vietj vietj added this to the 4.5.12 milestone Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants