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

BlazingMQ Client/Broker Protocol #95

Closed

Conversation

quarter-note
Copy link
Contributor

Issue number of the reported bug or feature request: #28

Describe your changes
Attempting to document the client/broker protocol in BlazingMQ. At a high level, document will cover three areas:

  • BlazingMQ wire protocol
  • Message exchanges b/w client and broker
  • Guidelines and recommendations for client library implementation

@quarter-note quarter-note added the documentation Improvements or additions to documentation label Aug 22, 2023
@quarter-note quarter-note self-assigned this Aug 22, 2023
@quarter-note quarter-note changed the title [WIP] BlazingMQ Client/Broker Protocol BlazingMQ Client/Broker Protocol Aug 24, 2023
@quarter-note quarter-note marked this pull request as ready for review August 24, 2023 18:25
@quarter-note quarter-note requested a review from a team as a code owner August 24, 2023 18:25
@quarter-note
Copy link
Contributor Author

Hi @sgalichkin Could you review the document? I am asking you to review since you are familiar with quite a bit internal SDK details including the wire protocol. Thanks.

@quarter-note quarter-note mentioned this pull request Aug 24, 2023
1 task
@quarter-note quarter-note added the A-Docs Area: Documentation label Aug 24, 2023
@kafkiansky
Copy link

Personally, I really missed the detailed layout of all the messages, especially the binary ones. For example, if you look at the Kafka protocol documentation, you will see a schema of all messages, including their types, order, purpose, changes from version to version, and so on. I have to reverse-engineering your Java SDK a lot to understand how to encode binary messages. Maybe when I'm done with my SDK, I'll make a pull-request with the binary message schema added. Anyway thanks a lot for your work, for the first version of the protocol documentation it is very good.

Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

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

Idea typo search

docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
@quarter-note quarter-note mentioned this pull request Sep 1, 2023
1 task
Copy link
Collaborator

@pniedzielski pniedzielski left a comment

Choose a reason for hiding this comment

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

Mostly proofreading comments. In a few places, it seems like some fairly important details are hidden deep into the document, which may benefit from having them lifted out earlier. See comments for details.

docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
docs/docs/architecture/client_broker_protocol.md Outdated Show resolved Hide resolved
@quarter-note
Copy link
Contributor Author

I have applied commit suggestions. Will address general comments later. Thanks of reviewing @678098 and @pniedzielski

quarter-note and others added 14 commits December 7, 2023 15:34
Signed-off-by: Ankur Saxena <[email protected]>
quarter-note and others added 5 commits December 7, 2023 15:48
Co-authored-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Ankur Saxena <[email protected]>
Co-authored-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Ankur Saxena <[email protected]>
Co-authored-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Ankur Saxena <[email protected]>
Co-authored-by: Patrick M. Niedzielski <[email protected]>
Signed-off-by: Ankur Saxena <[email protected]>
Signed-off-by: Ankur Saxena <[email protected]>
@quarter-note
Copy link
Contributor Author

Replaced w/ #167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Docs Area: Documentation documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants