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

THROTTLED status when journal utilization is over threshold #1952

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@mikkolehtisalo
Contributor

mikkolehtisalo commented Mar 20, 2016

Implementation of #1100.

Quick notes:

  • The feature is disabled by default
  • Adds THROTTLED load balancer status with 429 TOO MANY REQUESTS (meant for throttling purposes).
  • Evaluates the need to throttle at journal cleanups (alternatively could be also at metrics)
  • Threshold percentage configured via new configuration value lb_throttle_threshold_percentage
  • Should not break existing load balancer configurations, because they commonly consider anything besides ALIVE response as DEAD
  • Needs to be documented here, if gets accepted

Please review and comment :)

@lennartkoopmann

This comment has been minimized.

Member

lennartkoopmann commented Mar 20, 2016

I did not review the code but I like the idea! Thanks Mikko.

@bernd bernd added the feature label Mar 22, 2016

@bernd bernd added this to the 2.1.0 milestone Mar 22, 2016

@bernd

This comment has been minimized.

Member

bernd commented Mar 22, 2016

@mikkolehtisalo Thank you for the PR! 👍 It's a bit late to put this in 2.0 unfortunately but we will review this for 2.1.

@mikkolehtisalo

This comment has been minimized.

Contributor

mikkolehtisalo commented Mar 22, 2016

Okay. I'll be glad to change it based on the feedback. I am still trying to catch on some things...

@joschi

This comment has been minimized.

Contributor

joschi commented May 24, 2016

@mikkolehtisalo Could you please rebase your PR on the current master branch?

@joschi joschi self-assigned this May 24, 2016

MetricRegistry metricRegistry) {
MetricRegistry metricRegistry,
ServerStatus serverStatus,
Configuration configuration) {

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

Please also use the named property here:

@Named("lb_throttle_threshold_percentage") int throttleThresholdPercentage

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 29, 2016

Contributor

Fixed.

* Implementing StatusType for 429 TOO MANY REQUEST (the most suitable HTTP status code for
* requesting throttling), because javax.ws.rs.core.Response.Status does not implement it.
*/
private class TooManyRequestsStatus implements Response.StatusType {

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

Please move this as a self-contained class into the org.graylog2.rest package.

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 29, 2016

Contributor

Moved.

public Response.Status.Family getFamily() { return Response.Status.Family.CLIENT_ERROR; }
@Override
public String getReasonPhrase() { return "TOO MANY REQUESTS"; }

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

"Too Many Requests" should be fine. No need to yell. 😉

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 29, 2016

Contributor

Copy & paste... Fixed.

@@ -101,6 +101,9 @@
@Parameter(value = "lb_recognition_period_seconds", validator = PositiveIntegerValidator.class)
private int loadBalancerRecognitionPeriodSeconds = 3;
@Parameter(value = "lb_throttle_threshold_percentage", validator = PositiveIntegerValidator.class)
private int loadBalancerThrottleThresholdPercentage = 110; // Over 100% is effectively disabled

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

I think setting it to 100 or even 95 by default is fine.

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 29, 2016

Contributor

Ok. Changed accordingly.

@@ -102,6 +105,8 @@
public static final long DEFAULT_COMMITTED_OFFSET = Long.MIN_VALUE;
public static final int NOTIFY_ON_UTILIZATION_PERCENTAGE = 95;
private static ServerStatus serverStatus;
private static Configuration configuration;

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

Why use static variables here?

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 29, 2016

Contributor

Not static anymore.

* journal utilization percentage. As the utilization ratio is reliable only after cleanup,
* that's where this is called from.
*/
private void updateLoadBalancerStatus(double utilizationPercentage) {

This comment has been minimized.

@joschi

joschi May 24, 2016

Contributor

I'm a little bit uncomfortable with changing the server/load balancer status directly from this class.
An alternative could be emitting an event on the EventBus and a subscriber to that event which will change the serverStatus accordingly.

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 29, 2016

Contributor

I wondered about this. How about just using ServerStatus.publishLifecycle? Other parts of code seemed to use that.

With events, probably should check the status and send only actual changes. I would feel uneasy about EventBus unless cases of direct use would be removed. Mixing the two approaches might cause unforeseen consequences.

@@ -157,8 +162,12 @@ public KafkaJournal(@Named("message_journal_dir") File journalDirectory,
@Named("message_journal_max_age") Duration retentionAge,
@Named("message_journal_flush_interval") long flushInterval,
@Named("message_journal_flush_age") Duration flushAge,
MetricRegistry metricRegistry) {
@Named("lb_throttle_threshold_percentage") double throttleThresholdPercentage,

This comment has been minimized.

@joschi

joschi May 30, 2016

Contributor

This has to have the same type as in the configuration class (i. e. int here or double in the configuration class).

This comment has been minimized.

@mikkolehtisalo

mikkolehtisalo May 30, 2016

Contributor

Changed to int.

Adding THROTTLED load balancer status
Adding overriding load balancer status to THROTTLED

Implement returning THROTTLED status if set

Api doc to mention that also THROTTLED may be set

Added override status just in case

Configuration option lb_throttle_threshold_percentage for throttling threshold

Sample configuration option to graylog.conf

Flip the load balancer status to THROTTLED or ALIVE in cleanupSegmentsToMaintainSize based on the journal utilization percentage. Will not touch DEAD.

Added missing ServerStatus and Configuration variables

Added logging to automatic THROTTLING, protect from NPE if KafkaJournal was not initialized for some reason

Clarified that the feature is disabled if configuration value is not set

Fixes

Fix tests

Change to private

Changed lb_throttle_threshold_percentage to be int

@mikkolehtisalo mikkolehtisalo deleted the mikkolehtisalo:issue-1100 branch May 30, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented May 31, 2016

@mikkolehtisalo Any reason you've closed this PR and deleted your branch? I think it was very useful.

@mikkolehtisalo mikkolehtisalo restored the mikkolehtisalo:issue-1100 branch May 31, 2016

@mikkolehtisalo

This comment has been minimized.

Contributor

mikkolehtisalo commented May 31, 2016

I think the idea might not be worth pursuit, because it increases complexity. However, feel free to work on it if you think it is worth the time.

joschi added a commit that referenced this pull request May 31, 2016

Throttle LB status if journal utilization is too high
This change set adds the load balancer status "THROTTLED", represented by HTTP status 429 (Too many requests).

The load balancer status will be changed to THROTTLED (HTTP 429) if the journal utilization exceeds a configured
threshold, see `lb_throttle_threshold_percentage` setting, and will be changed to ALIVE (HTTP 200) if the utilization
is lower again.

Originally written by Mikko Lehtisalo (https://github.com/mikkolehtisalo)

Closes #1100
Refs #1952

joschi added a commit that referenced this pull request May 31, 2016

Throttle LB status if journal utilization is too high
This change set adds the load balancer status "THROTTLED", represented by HTTP status 429 (Too many requests).

The load balancer status will be changed to THROTTLED (HTTP 429) if the journal utilization exceeds a configured
threshold, see `lb_throttle_threshold_percentage` setting, and will be changed to ALIVE (HTTP 200) if the utilization
is lower again.

Originally written by Mikko Lehtisalo (https://github.com/mikkolehtisalo)

Closes #1100
Refs #1952

@mikkolehtisalo mikkolehtisalo reopened this Jun 1, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented Jun 1, 2016

@mikkolehtisalo I've continued to work on this in #2312, so I'll close this PR again. Comments in the new PR are welcome!

@joschi joschi closed this Jun 1, 2016

joschi added a commit that referenced this pull request Jun 7, 2016

Throttle LB status if journal utilization is too high
This change set adds the load balancer status "THROTTLED", represented by HTTP status 429 (Too many requests).

The load balancer status will be changed to THROTTLED (HTTP 429) if the journal utilization exceeds a configured
threshold, see `lb_throttle_threshold_percentage` setting, and will be changed to ALIVE (HTTP 200) if the utilization
is lower again.

Originally written by Mikko Lehtisalo (https://github.com/mikkolehtisalo)

Closes #1100
Refs #1952

dennisoelkers added a commit that referenced this pull request Jun 7, 2016

Throttle LB status if journal utilization is too high (#2312)
This change set adds the load balancer status "THROTTLED", represented by HTTP status 429 (Too many requests).

The load balancer status will be changed to THROTTLED (HTTP 429) if the journal utilization exceeds a configured
threshold, see `lb_throttle_threshold_percentage` setting, and will be changed to ALIVE (HTTP 200) if the utilization
is lower again.

Originally written by Mikko Lehtisalo (https://github.com/mikkolehtisalo)

Closes #1100
Refs #1952
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment