SocketOptions configuration is missing - Need to set tcp read/write buffer sizes #473
Conversation
…ite buffer size from 32KB to 128 KB.
|
Linux test FAILed. |
|
Windows test FAILed. |
| HazelcastServer instance(*g_srvFactory); | ||
|
|
||
| ClientConfig clientConfig; | ||
| clientConfig.getNetworkConfig().getSocketOptions().setKeepAlive(false).setReuseAddress(true).setTcpNoDelay(false).setLingerSeconds(5).setBufferSize(2 * 1024); |
There was a problem hiding this comment.
This is not testing the functionality of each option but just a test to see that the client works when the options are set. This was mostly for coverage. More meaningful functionality tests would be better but I did not come up with a way how to test tcp level such settings yet.
| class HAZELCAST_API SocketOptions { | ||
| public: | ||
| /** | ||
| * constant for kilobyte |
There was a problem hiding this comment.
I think we dont need this comment, it doesnt say more than the variable name
There was a problem hiding this comment.
These are from Java client documentation. If needed we need to change both. I think it can stay.
There was a problem hiding this comment.
I think they are not even used in Java :) I sent a cleanup PR for that https://github.com/hazelcast/hazelcast/pull/14147/files
There was a problem hiding this comment.
As they are part of API we cant change them I guess. If there isnt any check like checkstyle for CPP we shouldnt have them here. They are probably there in Java because of checkstyle(otherwise the build fails).
| static const int KILO_BYTE = 1024; | ||
|
|
||
| /** | ||
| * default buffer size of Bytes |
There was a problem hiding this comment.
| int getLingerSeconds() const; | ||
|
|
||
| /** | ||
| * Enable/disable SO_LINGER with the specified linger time in seconds |
There was a problem hiding this comment.
What value disables SO_LINGER?
There was a problem hiding this comment.
Added doc:
*
* if set to a value of 0 or less then it is disabled.
*
* Default value is 3.
| SocketOptions &setLingerSeconds(int lingerSeconds); | ||
|
|
||
| /** | ||
| * Gets the SO_SNDBUF and SO_RCVBUF options to the specified value in KB |
There was a problem hiding this comment.
Gets the SO_SNDBUF and SO_RCVBUF options value
| * Gets the SO_SNDBUF and SO_RCVBUF options to the specified value in KB | ||
| * @return bufferSize KB value | ||
| */ | ||
| int getBufferSize() const; |
There was a problem hiding this comment.
If it returns in KB the name of the function can be getBufferSizeInKB
There was a problem hiding this comment.
it is actually bytes. Corrected the documentation now.
| * @param bufferSize KB value | ||
| * @return SocketOptions configured | ||
| */ | ||
| SocketOptions &setBufferSize(int bufferSize); |
There was a problem hiding this comment.
It is bytes. Changed doc.
There was a problem hiding this comment.
if it is bytes I think method name should be setBufferSizeInBytes
| } | ||
|
|
||
| SocketInterface &TcpSocket::setSocketOptions(const client::config::SocketOptions &socketOptions) { | ||
| int optionValue = socketOptions.getBufferSize(); |
There was a problem hiding this comment.
I think for better readability instead of using optionValue we can use directly the option's name, for example here int bufferSizeInKB = socketOptions.getBufferSizeInKB()
There was a problem hiding this comment.
I named it this way to reuse the same local variable for each option instead of stack allocating a new one for each.
|
Linux test FAILed. |
|
Windows test FAILed. |
…xist at Linux. Also some review comment fixes.
ihsandemir
force-pushed
the
socketOptionsConfig
branch
from
47e26fb to
129103c
Compare
|
Windows test FAILed. |
|
Linux test FAILed. |
…uring SSL connect or after socket call for TcpSocket).
|
Windows test FAILed. |
|
Windows test FAILed. |
|
Linux test PASSed. |
|
Linux test PASSed. |
|
Windows test PASSed. |
|
Linux test PASSed. |
|
Windows test PASSed. |
|
Linux test PASSed. |
|
Linux test FAILed. |
|
Windows test FAILed. |
|
Linux test PASSed. |
|
Windows test PASSed. |
4 participants
Added
SocketOptionsconfiguration and increased default tcp read/write buffer size from 32KB to 128 KB.fixes #465