Add server-side lookup throttling #181
Conversation
rdhabalia
added
the
type/enhancement
| } | ||
| return null; | ||
| }); | ||
| if (service.getLookupRequestSemaphore().tryAcquire()) { |
There was a problem hiding this comment.
To avoid indenting the whole section, what about doing:
if (!service.getLookupRequestSemaphore().tryAcquire()) {
/// deal with error
return;
}
// continue normalThere was a problem hiding this comment.
Actually, here I have replaced thenAccept/exceptionally with handle(data, ex) so, don't have to keep duplicate service.getLookupRequestSemaphore().release(). So, this change anyway changed entire section so, kept into if/else.
| return null; | ||
| }); | ||
| } else { | ||
| log.warn("[{}] Failed Partition-Metadata lookup due to too many lookup-requets {}", remoteAddress, topic); |
| private int getLookupRequestPermits() { | ||
| int pendingLookupRequest = pulsar.getConfiguration().getMaxConcurrentLookupRequest(); | ||
| try { | ||
| Optional<Map<String, String>> data = dynamicConfigurationCache.get(LOOKUP_THROTTLING_PATH); |
There was a problem hiding this comment.
Dynamic configuration of the parameter should go in separate PR
There was a problem hiding this comment.
is that fine if we remove this configuration-update once other PR is created?
There was a problem hiding this comment.
Either that, or just use the value from config file as it is (like any other config parameter) and then we can open a separate issue to discuss the broader dynamic config approach.
There was a problem hiding this comment.
yes, we can read from config-file but part of this PR also requires to test updating semaphore at runtime and if config value gets changed then we are also updating lookupRequestSemaphore so, even with Dynamic-config-change we might need this listener logic and we have to provide a way to inject listener on config value change in dynamic-config-PR.
So, should we keep this PR open until we come up with the approach as anyway it should be part of 1.17 release.
rdhabalia
force-pushed
the
server_throttle
branch
from
4146fb0 to
ce6cae9
Compare
rdhabalia
force-pushed
the
server_throttle
branch
from
ce6cae9 to
8eeb3f1
Compare
rdhabalia
force-pushed
the
server_throttle
branch
4 times, most recently
from
1fa35e6 to
aad5acc
Compare
rdhabalia
force-pushed
the
server_throttle
branch
from
aad5acc to
7fcafdc
Compare
| @@ -81,6 +81,7 @@ enum ServerError { | |||
| TopicNotFound = 11; // Topic not found | |||
| SubscriptionNotFound = 12; // Subscription not found | |||
| ConsumerNotFound = 13; // Consumer not found | |||
| TooManyRequest = 14; // Error with too many simultaneously request | |||
There was a problem hiding this comment.
Minor thing: this should be plural: TooManyRequests
fix apache#181 This pull request fix the deadlock in kop.
Motivation
Sometimes, broker requires a way to throttle newly incoming traffic and requires to restrict number of incoming concurrent lookup requests.
Modifications
Result
Broker can have capability to throttle newly incoming traffic by restricting number of concurrent lookup request.