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

Added a new parameter for configuring socket connection. #71

Merged
merged 4 commits into from
Jul 12, 2021

Conversation

xydan83
Copy link
Contributor

@xydan83 xydan83 commented Jul 12, 2021

From #70 issue.

    # By default, in order to optimize the transmission of small data packets,
    # the send buffer size is set to 2048 bytes. To transfer large data packets (>50MB),
    # it is recommended to increase the buffer value.
    # It is also possible to set all possible socket settings.
    # See https://docs.python.org/3/library/socket.html#socket.socket.setsockopt for more information.
    # If the socket_options is None, then the system defaults are used.

arozhkov added 2 commits July 6, 2021 11:22
…he timeout in seconds before a message with QoS>0 is retried. 20 seconds by default.
Copy link
Member

@frederikaalund frederikaalund left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. This is a really cool feature and a nice addition to asyncio-mqtt. 👍

I've reviewed the changes and I'd like some minor changes here and there. Let me know what you think. :)

# It is also possible to set all possible socket settings.
# See https://docs.python.org/3/library/socket.html#socket.socket.setsockopt for more information.
# If the socket_options is None, then the system defaults are used.
socket_options: Optional[Tuple[int, int, Union[int, bytes]]] = (socket.SOL_SOCKET, socket.SO_SNDBUF, 2048),
Copy link
Member

Choose a reason for hiding this comment

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

Good job with the initial typing here. 👍 I really like the details in the comment as well

I suggest that we drop the default SO_SNDBUF setting. Move it to the comment, though, as a nice example.

I also suggest that we expand on this, so that people can set multiple options using any of the three overloads of setsockopt. First, we define:

# See the overloads of `socket.setsockopt` for details.
SocketOption = Union[
    Tuple[int, int, Union[int, bytes]],
    Tuple[int, int, None, int],
]

Second, we accept an iterable of socket options in the Client.__init__.

socket_options: Iterable[SocketOption],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, is good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found it worth adding one more thing so that there would be no compatibility issues with the new library version when upgrading.
If we do this: socket_options: Iterable[SocketOption], then we get an error
TypeError: init() missing 1 required keyword-only argument: 'socket_options', because we have no input parameter when constructing an object.

Maybe add a default value?
For example: socket_options: Optional[Iterable[SocketOption]] = ()

asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
asyncio_mqtt/client.py Outdated Show resolved Hide resolved
@xydan83
Copy link
Contributor Author

xydan83 commented Jul 12, 2021

Thank you! Great changes. Thus, the setting will be more flexible.

@frederikaalund
Copy link
Member

Glad you like it. :) Let me know when you push the new changes. 👍

For now, there seems to be some merge conflicts. I recommend that you:

  • Rebase on top of master git pull --rebase -i (the -i is for interactive mode)
  • Force push your changes git push --force

That should also get rid of the "Added new parameter 'message_retry_set' to the Client._" commit that we already merged to master. :)

arozhkov and others added 2 commits July 12, 2021 18:44
@xydan83
Copy link
Contributor Author

xydan83 commented Jul 12, 2021

I applied the changes, check please:)

@frederikaalund frederikaalund merged commit 34685f1 into sbtinstruments:master Jul 12, 2021
@frederikaalund
Copy link
Member

I merged your pull request. Thanks for you contribution to asyncio-mqtt. :) 👍

@xydan83
Copy link
Contributor Author

xydan83 commented Jul 12, 2021

I merged your pull request. Thanks for you contribution to asyncio-mqtt. :)

Thanks for a great project! ))

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.

None yet

2 participants