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

WebService refactoring #40

Merged
merged 6 commits into from
Sep 27, 2016
Merged

WebService refactoring #40

merged 6 commits into from
Sep 27, 2016

Conversation

radekg
Copy link
Contributor

@radekg radekg commented Sep 27, 2016

Motivation

Clarifying the code in the WebService.

Modifications

Removes the unnecessary argument in the constructor.
Removes magic values.

Result

No functional changes.

Little WebService refactor, removing magic values, removing unnecessa…
@yahoocla
Copy link

CLA is valid!

Copy link

@agarman agarman left a comment

Choose a reason for hiding this comment

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

I don't see any issue with changes. Pulsar's config is pulled out by AuthenticationFilter, which sets a precedent that that's acceptable. I defer to @merlimat & friends thought.

tlsConnector.setHost(pulsar.getBindAddress());
connectors.add(tlsConnector);
}

// Limit number of concurrent HTTP connections to avoid getting out of file descriptors
connectors.stream().forEach(c -> c.setAcceptQueueSize(1024 / connectors.size()));
connectors.stream().forEach(c -> c.setAcceptQueueSize(WebService.MAX_CONCURRENT_REQUESTES / connectors.size()));
Copy link

Choose a reason for hiding this comment

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

While in here... is there a good reason for connectors.stream().forEach vs. just using connectors.forEach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Also the line below, as we're iterating over the connectors, while not to call server.addConnector(c)?

Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

LGTM

@radekg
Copy link
Contributor Author

radekg commented Sep 27, 2016

Apologies, fix coming, seems one can't simply addConnector. Running tests on the fork.

Turns out one can't simply addConnector...
@merlimat merlimat merged commit 0577fda into apache:master Sep 27, 2016
sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018
* Create pulsar-functions module (#1)

* Create pulsar-functions module

* rename `sdk` package to `api`

* Added the first cut of the Java interface for Pulsar functions (#2)

* Reconciled the interface between worker and client with Jerry

* Fixed the update method
massakam pushed a commit to massakam/pulsar that referenced this pull request Aug 6, 2020
hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021
- Add some test cases for KafkaRequestHandler. fix error in the tests.
- change handleListOffsetRequest to get topic offset from PersistentTopic directly instead of calling  admin command to get topic stats.
- In topicManager add a Map to cache PersistentTopic.
dlg99 added a commit to dlg99/pulsar that referenced this pull request Mar 21, 2022
(cherry picked from commit c823c08)
(cherry picked from commit c17cd00)

Co-authored-by: Rui Fu <[email protected]>
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