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

ConnectionString javadoc #84

Merged
merged 5 commits into from
Aug 29, 2024

Conversation

surovenko
Copy link
Contributor

Also I have changed param clientMaxPoolSize from 10 to 5000 because http client from one-nio usually used for high-load system where we need a lot of connections and value 5000 more suitable for such cases. And because of poor docs it's not obvious how to increase this value. I bet a lot of developers like me starting to use http client on production and realised that the performance is terrible, although all integration tests were normal, since 10 sockets per address are enough for tests.

Copy link
Member

@incubos incubos left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution, but there are some suggestions.

@@ -33,7 33,7 @@ public class SocketPool extends Pool<Socket> implements SocketPoolMXBean {

public SocketPool(ConnectionString conn) {
super(conn.getIntParam("clientMinPoolSize", 0) > 0 ? 1 : 0,
conn.getIntParam("clientMaxPoolSize", 10),
conn.getIntParam("clientMaxPoolSize", 5000),
Copy link
Member

Choose a reason for hiding this comment

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

Altering defaults is extremely hazardous in such projects. Please, revert this change.

Comment on lines 38 to 58
* keepalive | default true
* bufferSize | default 8000
* timeout | default 3000
* readTimeout | default 3000
* connectTimeout | default 1000
* fifo | default false
* jmx | default false
* clientMinPoolSize | default 0
* clientMaxPoolSize | default 5000
* schedulingPolicy | default OTHER | @see SchedulingPolicy
* tos | default 0 | @see Socket Type of Service
* recvBuf | default 0
* sendBuf | default 0
* backlog | default 128
* selectors | default 0
* minWorkers | default 0
* maxWorkers | default 0
* queueTime | default 0
* closeSessions | default false
* threadPriority | default Thread.NORM_PRIORITY
* proxy
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding monospaced formatting?

@surovenko
Copy link
Contributor Author

@incubos not sure what "monospaced formatting" is. Could you please provide some example with this? Can't find any in google. Thanks!

@incubos
Copy link
Member

incubos commented Jun 29, 2024

@incubos not sure what "monospaced formatting" is. Could you please provide some example with this? Can't find any in google. Thanks!

Here is some example, otherwise the formatting will be unaligned.

@surovenko
Copy link
Contributor Author

@incubos done

Copy link
Member

@incubos incubos left a comment

Choose a reason for hiding this comment

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

New unit test doesn't pass:

[ERROR] Failures: 
[ERROR]   ConnectionStringTest.testConnectionStrings:57 expected:<[def]> but was:<[?]>

@surovenko
Copy link
Contributor Author

@incubos fixed

@incubos incubos self-assigned this Aug 29, 2024
Copy link
Member

@incubos incubos left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@incubos incubos merged commit e3a8cbf into odnoklassniki:master Aug 29, 2024
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.

2 participants