Skip to content

Conversation

@aogburn
Copy link
Collaborator

@aogburn aogburn commented Jul 6, 2022

@KarmBot
Copy link
Collaborator

KarmBot commented Jul 6, 2022

Can one of the admins verify this patch? If it is not dangerous to our systems, juts tell me to "run_tests". ( without the underscore and quotes 😃 )

@rhusar rhusar requested a review from jfclere July 7, 2022 10:42
@rhusar
Copy link
Member

rhusar commented Jul 7, 2022

@jfclere Please review. Also, this will probably need a forward-port PR to 2.x.

Copy link
Member

@jfclere jfclere left a comment

Choose a reason for hiding this comment

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

Please fix and make sure you follow the code convention (c and not C++) don't create variable in the middle of the code that won't compile with the minimum compiler.

continue; /* wrong shared memory address */
if (PROXY_WORKER_IS_USABLE(worker)) {
if (best == NULL)
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

that will break the logic (well the trace and a set), better break here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is moving context_host_ok logic into internal_find_best_byrequests. context_host_ok already returned null if best from find_node_context_host is null here. So instead of repeating context_host_ok/find_node_context_host each iteration, find_node_context_host is called just once. So if best is null in one iteration, it would be null in all if we did a break to repeat iterations. Thus it seems we should just return here.

Copy link
Member

Choose a reason for hiding this comment

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

We missed the piece

no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes; sorry, see what you mean now.

@jfclere
Copy link
Member

jfclere commented Aug 3, 2022

There are other place where context_host_ok() is used should we also look to them?
According to my tests with upstream the proposed PR doesn't help much to reduce httpd load, how did you test it?

@aogburn
Copy link
Collaborator Author

aogburn commented Aug 3, 2022

My comments on https://issues.redhat.com/browse/MODCLUSTER-757 describe the tests and results. I simulated 500 balancer members in those tests and just did simple load tests from ab without any session cookie in the requests. Ensure any test load does not use or provide a session that could be stickied as stickied requests avoid internal_find_best_byrequests calls.

@aogburn
Copy link
Collaborator Author

aogburn commented Aug 3, 2022

Checking other places where context_host_ok/find_node_context_host, may be called, I see:

  1. proxy_cluster_pre_request->find_session_route->find_route_worker->context_host_ok/find_node_context_host
  2. proxy_cluster_trans->get_route_balancer->find_node_context_host
  3. proxy_cluster_trans->get_context_host_balancer->find_node_context_host

Those may be worth following up on if we want to get even more optimal performance but it should provide a much smaller performance gain compared to this in the end. But I filed https://issues.redhat.com/browse/MODCLUSTER-764 to track those points.

@jfclere jfclere merged commit 1043c74 into modcluster:1.3.x Aug 15, 2022
@aogburn aogburn deleted the MODCLUSTER-757 branch February 15, 2023 16:18
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.

4 participants