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

[improve] [broker] Servlet support response compression #21667

Conversation

hangc0276
Copy link
Contributor

@hangc0276 hangc0276 commented Dec 4, 2023

Fixes #16321

Main Issue: #xyz

PIP: #xyz

Motivation

The original PR is #18102, after communicating with the author @xuesongxs, I opened a new PR to handle it.

I tested the Prometheus metric with 1000 topics and enabled the compression to get 33x data saving.

# curl -H 'Accept-Encoding: gzip' -o kk0.log.gz http://localhost:8080/metrics/ 
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  927k    0  927k    0     0  1910k      0 --:--:-- --:--:-- --:--:-- 1941k
# curl -o kk1.log http://localhost:8080/metrics/  
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 33.2M    0 33.2M    0     0  73.1M      0 --:--:-- --:--:-- --:--:-- 74.4M
# ll -lrt |grep kk
-rw-r--r--     1 hangc  staff   928K Nov 28 18:46 kk0.log.gz
-rw-r--r--     1 hangc  staff    33M Nov 28 18:46 kk1.log

Modifications

Add gzip compression by default and the whether the response is compressed or not is based on the client request header Accept-Encoding: gzip.
The feature will improve the metric cross-zone traffic a lot especially Pulsar broker deployed on Cloud.

This is a new feature and it will be released on the feature branch.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@hangc0276 hangc0276 self-assigned this Dec 4, 2023
@hangc0276 hangc0276 added this to the 3.2.0 milestone Dec 4, 2023
@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Dec 4, 2023
@hangc0276 hangc0276 requested a review from merlimat December 4, 2023 16:55
@hangc0276
Copy link
Contributor Author

@asafm @lhotari Please help take a look if you have time, thanks a lot.

@lhotari lhotari changed the title [improve] [broker] Serverlet support response compression [improve] [broker] Servlet support response compression Dec 5, 2023
@hangc0276 hangc0276 requested review from horizonzy and zymap December 5, 2023 07:41
@lhotari
Copy link
Member

lhotari commented Feb 27, 2024

@hangc0276 please address the review comments. it would be great to finally get this merged after a few years of reviews :)

@hangc0276
Copy link
Contributor Author

@hangc0276 please address the review comments. it would be great to finally get this merged after a few years of reviews :)

Sure, I will address it.

@hangc0276 hangc0276 force-pushed the chenhang/serverlet_support_response_compression branch from 782f2c9 to c3f6449 Compare March 12, 2024 15:58
@Technoboy- Technoboy- dismissed asafm’s stale review March 13, 2024 05:19

all resloved

@lhotari
Copy link
Member

lhotari commented Mar 27, 2024

Cherry-picking to branch-3.0 and branch-3.2 since the lack of compression is causing issues when metrics are enabled and there are a large amount of active topics.

@lhotari
Copy link
Member

lhotari commented Mar 27, 2024

Btw. It looks like this change could be improved. Instead of adding multiple GzipHandler instances, it would be better to add a single instance and provide a configuration option for configuring excluded paths, just in case the compression causes problems for some cases.

@lhotari
Copy link
Member

lhotari commented Mar 27, 2024

Early WIP PR for improvement: #22370

lhotari pushed a commit to lhotari/pulsar that referenced this pull request Apr 15, 2024
(cherry picked from commit 7a4e16a)

# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Apr 15, 2024
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Apr 15, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 16, 2024
(cherry picked from commit 7a4e16a)

(cherry picked from commit 93664d7)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
(cherry picked from commit 7a4e16a)

(cherry picked from commit 93664d7)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
(cherry picked from commit 7a4e16a)

(cherry picked from commit 93664d7)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
(cherry picked from commit 7a4e16a)

(cherry picked from commit 93664d7)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
(cherry picked from commit 7a4e16a)

(cherry picked from commit 93664d7)
nodece pushed a commit to nodece/pulsar that referenced this pull request Jul 24, 2024
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gzip compression before http service response
6 participants