Add client and server lookup throttling #176

rdhabalia · 2017-01-27T01:37:45Z

Motivation

Sometimes, broker requires a way to throttle newly incoming traffic and requires to restrict number of incoming concurrent lookup requests. It is also useful to throttle at client in order to avoid large number of concurrent lookup-requests while creating producers/consumers.

Modifications

Add broker side throttling to serve max allowed concurrent lookup request
Add client side lookup throttling
Update value of maxConcurrentLookupRequest dynamically at broker
AdminApi command to update maxConcurrentLookupRequest

Result

Broker can have capability to throttle newly incoming traffic by restricting number of concurrent lookup request.

saandrews · 2017-01-30T18:10:19Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/BrokerService.java

+        } catch (Exception e) {
+            log.warn("Got exception when reading ZooKeeper path [{}]:", LOOKUP_THROTTLING_PATH, e);
+        }
+        return pendingLookupRequest;


Add a debug log with the actual value picked up?

added debug log.

saandrews · 2017-01-30T18:25:38Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/lookup/DestinationLookup.java


 import io.netty.buffer.ByteBuf;
+import io.swagger.annotations.ApiOperation;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;

 @Path("/v2/destination/")
 @NoSwaggerDocumentation


Should we not throttle http lookup?

added http-throttling change as well.

merlimat

A couple of comments:

The implementation for client side and broker side throttling is very different and ideally it would be better to break this down into 2 separate commits.
While I agree that having variables dynamically tunable at runtime (without restarting the broker) is a great thing, we shouldn't be doing that on a case by case, adding the REST handlers (that would not be possible to remove later.. ). We should address the dynamic configuration in a separate PR, using a more general approach

merlimat · 2017-01-30T23:49:33Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/lookup/DestinationLookup.java

+    @ApiOperation(value = "Update allowed concurrent lookup permits. This operation requires Pulsar super-user privileges.")
+    @ApiResponses(value = { @ApiResponse(code = 204, message = "Lookup permits updated successfully"),
+            @ApiResponse(code = 403, message = "You don't have admin permission to create the cluster") })
+    public void createCluster(@PathParam("permits") int permits) {


Method should not be called createCluster()

rdhabalia · 2017-01-31T00:36:59Z

The implementation for client side and broker side throttling is very different and ideally it would be better to break this down into 2 separate commits.

Agree. I will create separate commit with different PR to address it.

While I agree that having variables dynamically tunable at runtime (without restarting the broker) is a great thing, we shouldn't be doing that on a case by case, adding the REST handlers (that would not be possible to remove later.. ). We should address the dynamic configuration in a separate PR, using a more general approach

Agree. Does it make sense to create a separate resource (REST controller) Configuration which allows to add/update/delete configuration: param1: zk_path, body. json_value

merlimat · 2017-01-31T00:45:29Z

Agree. Does it make sense to create a separate resource (REST controller) Configuration which allows to add/update/delete configuration: param1: zk_path, body. json_value

We need to think well how to expose the whole thing in consistent way. You have the values from the config file + the values in ZK. There should be a clear way to understand what is the actual configuration that is:

Supposed to be applied to all brokers
It's actually being applied in a given broker...

Other problem is how to clearly identify which values are dynamically tunable (eg: zk servers string will not be dynamic...)

rdhabalia · 2017-01-31T02:41:09Z

@merlimat @saandrews this PR was having 3 changes

server throttling
client throttling
updating throttling-configuration using REST-Api/AdminApi

I have created separate PR for #181(server) and #182(client).
As @merlimat mentioned: For dynamic-configuration update: we can separate PR to address generic configuration update.

merlimat · 2017-02-07T22:39:55Z

Closing in favor of #181

apache#176)

* PIP-55: Refresh auth credentials * Fixed lint issues

**Fixes:** apache#166 apache#153 apache#99 **Issue:** KoP uses [Kafka-2.0.0](https://github.com/streamnative/kop/blob/78d9ba3487d4d7c85a5d667d45d9b38aaa7c824f/pom.xml#L46) which supports [API_VERSION's](https://kafka.apache.org/protocol.html#The_Messages_ApiVersions) **0** --> **2** When **_Kafka-Clients-2.4.x+_**(using `API_VERSION: 3`) connects to KoP, it panics and following error stack is observed: `10:22:23.281 [pulsar-io-22-4] ERROR io.streamnative.pulsar.handlers.kop.KafkaCommandDecoder - error to get Response ByteBuf: java.lang.IllegalArgumentException: Invalid version for API key API_VERSIONS: 3 at org.apache.kafka.common.protocol.ApiKeys.schemaFor(ApiKeys.java:312) ~[?:?] at org.apache.kafka.common.protocol.ApiKeys.responseSchema(ApiKeys.java:286) ~[?:?] at org.apache.kafka.common.requests.ApiVersionsResponse.toStruct(ApiVersionsResponse.java:129) ~[?:?] at org.apache.kafka.common.requests.ResponseUtils.serializeResponse(ResponseUtils.java:40) ~[?:?]` **Resolved By:** Returning an `UNSUPPORTED_VERSION` [error-code: 35](https://kafka.apache.org/protocol.html#protocol_error_codes), which would notify the **_kafka-client_** to lower it's `API_VERSION`. As no list of `ApiKeys & versions` were available for the **kafka-clients** to refer, it safely falls-back to using `API_VERSION: 0` and KoP continues processing the kafka-messages using `API_VERSION: 0`. **Tested producing/consuming with Kafka-Clients:** > 2.0.0 2.2.2 2.3.1 2.4.0 2.4.1 2.5.0 2.5.1 2.6.0 **More...** KoP could have returned the list of supported `ApiKeys & versions` while sending the `UNSUPPORTED_VERSION` error-code, which would have made the **_kafka-client_** select the **_latest_** supported `API_VERSION` and use `API_VERSION: 2` instead of falling all the way back and using `API_VERSION: 0` Notes on how **_Kafka-Brokers_** is supposed to handle this scenario: > 2. On receiving ApiVersionsRequest, a broker returns its full list of supported ApiKeys and versions regardless of current authentication state (e.g., before SASL authentication on an SASL listener, do note that no Kafka protocol requests may take place on an SSL listener before the SSL handshake is finished). If this is considered to leak information about the broker version a workaround is to use SSL with client authentication which is performed at an earlier stage of the connection where the ApiVersionRequest is not available. Also, note that broker versions older than 0.10.0.0 do not support this API and will either ignore the request or close connection in response to the request. > > 3. If multiple versions of an API are supported by broker and client, clients are recommended to use the latest version supported by the broker and itself. _Reference: [Kafka-Protocol Guide](https://kafka.apache.org/protocol.html#api_versions)_ We analyzed how various **_Kafka-Brokers_** respond to a similar situation where the **_kafka-client's_** `API_VERSION` is higher than what is supported by the **_Kafka-Broker_**. ![Kafka-Broker-Client-Wireshark-Results](https://user-images.githubusercontent.com/63665447/91243701-34d3a500-e710-11ea-9752-f9980333ce1d.png) _Reference: Wireshark packet captures - [Kafka-Protocol-Study.zip](https://github.com/streamnative/kop/files/5127018/Kafka-Protocol-Study.zip)_ From the study we can infer that, in a similar `API_VERSION` mismatch scenario the **_Kafka-Brokers_** doesn't return the list of supported `ApiKeys & versions` when notifying the **_kafka-client_** with the `UNSUPPORTED_VERSION` [error-code: 35](https://kafka.apache.org/protocol.html#protocol_error_codes). Thus, forcing the **_kafka-clients_** to fall-back to using `API_VERSION: 0`. To keep KoP working in sync with the **_Kafka-Broker_** working, we decided not to return the list of supported `ApiKeys & versions`.

cherry-picked from apache#20121

rdhabalia added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Jan 27, 2017

rdhabalia added this to the 1.17 milestone Jan 27, 2017

rdhabalia self-assigned this Jan 27, 2017

rdhabalia force-pushed the throttle branch 4 times, most recently from 2e4ba2c to 4a3f388 Compare January 27, 2017 19:28

saandrews reviewed Jan 30, 2017

View reviewed changes

Add client and server lookup throttling

f6b21c8

rdhabalia force-pushed the throttle branch from 4a3f388 to f6b21c8 Compare January 30, 2017 20:26

merlimat reviewed Jan 30, 2017

View reviewed changes

add http-lookup throttling

2556f11

rdhabalia force-pushed the throttle branch from b35468c to 2556f11 Compare January 31, 2017 00:13

rdhabalia mentioned this pull request Jan 31, 2017

Introduce a way to update broker service configuration dynamically #184

Closed

merlimat closed this Feb 7, 2017

merlimat removed this from the 1.17 milestone Feb 7, 2017

sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018

Return a valid response upon not being able to get status from spawner (

3b7d1f4

apache#176)

hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020

PIP-55: Refresh auth credentials (apache#176)

6943327

* PIP-55: Refresh auth credentials * Fixed lint issues

xiaotongwang1 mentioned this pull request Aug 4, 2021

Pulsar 2.7.0+ KOP 2.7.2.x getPartitionedTopicMetadata timeout #11532

Closed

michaeljmarshall pushed a commit to michaeljmarshall/pulsar that referenced this pull request Apr 19, 2023

[fix][io] KCA: handle kafka sources that use commitRecord (apache#176 )

b47d8c6

cherry-picked from apache#20121

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add client and server lookup throttling #176

Add client and server lookup throttling #176

rdhabalia commented Jan 27, 2017

saandrews Jan 30, 2017

rdhabalia Jan 30, 2017

saandrews Jan 30, 2017

rdhabalia Jan 30, 2017

merlimat left a comment

merlimat Jan 30, 2017

rdhabalia commented Jan 31, 2017

merlimat commented Jan 31, 2017

rdhabalia commented Jan 31, 2017

merlimat commented Feb 7, 2017

Add client and server lookup throttling #176

Add client and server lookup throttling #176

Conversation

rdhabalia commented Jan 27, 2017

Motivation

Modifications

Result

saandrews Jan 30, 2017

Choose a reason for hiding this comment

rdhabalia Jan 30, 2017

Choose a reason for hiding this comment

saandrews Jan 30, 2017

Choose a reason for hiding this comment

rdhabalia Jan 30, 2017

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

merlimat Jan 30, 2017

Choose a reason for hiding this comment

rdhabalia commented Jan 31, 2017

merlimat commented Jan 31, 2017

rdhabalia commented Jan 31, 2017

merlimat commented Feb 7, 2017