feedback: Binary protocol | Tarantool #1661
Comments
|
Examples and illustrations are never necessary, they merely -- we hope -- make it easier to see exactly what is going back and forth. As for MessagePack, we refer to its "spec" page but we had to mention that there are slight modifications, and our list of constants is actually not from there. |
|
I agree, examples are useful, there are more useful if they are understandable. It think that the example is not because 1)it uses definitions that will be introduced two pages later 2)actual tcpdump (I tried) prints quite different output 3)as for me, tcpdump itself is not obvious thing 4)it invites to read a couple of dozens of lines of net_box sources. Actually the select protocol seems to be easier that this example of it. |
|
I would describe the protocol just in formatted JSON / YAML (with constant names instead of actual values), provide contant name-value table in a separate section and add a link to the msgpack standard. This way it should be as easy as possible to understand. Despite I know the protocol and the msgpack standard (not all codes, okay), it is hard for me to read the documentation. The protocol is much more simple comparing to the documentation. It is hard to defend 'simper' and 'harder' in a formal way. Let's just ask several more developers. @Mons, @kyukhin, @Korablev77, @Gerold103, guys, I would appreciate your feedback. |
|
alyapunov was correct that the tcpdump result was different from the illustration, I believe that is fixed now. |
|
Nope, most of time a developer work with an msgpack library, not 'with bytes'. Even if you implement the library yourself, you'll implement the library first and than work with integer/string/bytes/array/map — (almost) JSON types. Mixing of different abstraction levels will make the code harder to understand and maintain. And I propose to get rid of such mixing in our documentation. JSON/YAML is the human readable and well known representation of (almost) the same set of values as msgpack offers. The main purpose of the protocol description is to describe the protocol. Not msgpack. I wrote several tiny examples. They don't give exhaustive descriptions and they could be even imprecise or wrong. I just want to propose a syntax that is easy to read and translate to/from msgpack. Example (SQL insert without bindings): Request: <size>: msgpack(size(<header>) + size(<body>))
<header>: msgpack({
IPROTO_REQUEST_TYPE: IPROTO_EXECUTE,
IPROTO_SYNC: <integer>,
})
<body>: msgpack({
IPROTO_SQL_TEXT: <string>,
})Response: <size>: msgpack(size(<header>) + size(<body>))
<header>: msgpack({
IPROTO_SCHEMA_VERSION: <integer>,
IPROTO_STATUS_KEY: IPROTO_OK,
IPROTO_SYNC: <integer>,
})
<body>: msgpack({
IPROTO_SQL_INFO: {
IPROTO_SQL_INFO_ROW_COUNT: <integer>,
}
})Example of constant definitions:
|
|
During work on C++ connector I've faced troubles with documentation as well (had to scroll over the pages to find and understand the structure of request/response etc). It would be convenient having references (in tables or/and jsons) as Alexander proposed in the post above. |
|
I will make a large change trying to follow the suggestions above. I have assigned this to myself temporarily. |
|
I have changed the section: |
|
@NickVolynkin: See my comment above, from 19 days ago. |
|
@pgulutzan For some reason content of changed version looks not formatted: IMHO it's hard to read doc in such form (I'm using Google Chrome Version 80.0.3987.122 on Linux Mint 19.3 Cinnamon)... |
|
@Korablev77 we know about formatting and will fix it in the current sprint. |
|
If it is hard to read because of the text colour and font size: I acknowledged that this is not what Totktonada suggested, I explained "because the convention in our manual is to use italics not <...> for replaceable code". The way I did that is by concatenating :samp: and :code: on separate lines (the customized :codeitalic: etc. stuff was not working and probably wouldn't look much different). I believe NickVolynkin is referring to Issue#1801 Update formatting in "Binary protocol". Also, for the request to list all the IPROTO constants at the start, I did not put each constant on a separate line, so that people can scroll past it more quickly. Try to remember that the poor reader has already had to scroll past such a list because IPROTO constants are also in the "Index" at the top of the page, with separate lines. |
|
Hi Peter @pgulutzan
In my opinion, the new version is structurally better, and it solves the original issue. Although, formatting makes it harder to read. Fixing it (#1801) can be a good first issue for our new trainee, @xuniq. Of course, you're welcome to comment and review the new changes. |
|
@NickVolynkin: Okay, then we will not go back to the earlier version. |
I see, highlighting replaceable code is important in this document. @Onvember just helped me with a solution for this. We have a way to both highlight some text and have it in a code block: .. cssclass:: highlight
.. parsed-literal::
:samp:`box.space.{space-name}:create_index('{index-name}')`Output looks like this:
The highlighting with italics isn't very noticeable at this moment. We will fix it later. |
|
That is an intelligent suggestion, parsed-literal |
|
@pgulutzan we'll fix the styles soon, so that highlighting will look better. Maybe we'll use italics and color, or just color. For instance, I like this example:
The guideline doesn't mention it yet, right. Most of us didn't even know about parsed-literal until yesterday. :) |
|
In fact, I'd like to see the Binary protocol article restructured. However, this is for another issue. Closed. |
https://www.tarantool.io/en/doc/2.3/dev_guide/internals/box_protocol/#box-protocol-join
Неужели это все действительно нужно для того, чтобы понять примеры на этой страничке?. Мне кажется достаточно сказать, что это msgpack (линк msgpack.org) и дать пример этого msgpack.
The text was updated successfully, but these errors were encountered: