Skip to content

Fixes gh-2306 New field in binary iproto protocol #2356

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

Merged
merged 9 commits into from
Dec 13, 2021

Conversation

pgulutzan
Copy link
Contributor

@pgulutzan pgulutzan commented Sep 29, 2021

@EvgenyMekhanik:
Issue #2306 and issue #2307 and issue #2309 and issue #2310 involve
similar things and only one file, dev_guide/internals/box_protocol.rst.
I believed it was best to handle them together, so this commit is for all.
I am not handling the issues related to net_box.
That is why there is no :ref: link when I mention conn:new_stream().

I like the word "multiplex" but did not use it because the section starts by saying
"request multiplexing, for example ability to issue multiple requests asynchronously via the same connection"
so it could look odd to say we're adding it now.

I did not need to rename IPROTO_PROMOTE or IPROTO_DEMOTE etc. because they are undocumented,
I only needed to rename IPROTO_CONFIRM and IPROTO_ROLLBACK.

I notice in tarantool/tarantool#5860
that one of the objectives is:
"Interactive transactions. Such a transaction doesn’t need to be sent in one request - begin, commit and tx statements are allowed to be sent and executed in different requests."
This would be wonderful and would certainly be worth mentioning -- if it worked.
But I'm still seeing ER_TRANSACTION_YIELD using stream: via net.box with different requests.
Is this unfinished? Or am I misinterpreting that description of the objective?

I have not assigned you as a reviewer, please assign youself if you think it is appropriate.


Resolves #2306, #2307, #2309, #2310

@pgulutzan pgulutzan added the 2.10 label Sep 29, 2021
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 September 29, 2021 20:35 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 September 29, 2021 20:35 Inactive
@patiencedaur patiencedaur self-assigned this Oct 25, 2021
@maryartkey maryartkey self-assigned this Oct 25, 2021
@EvgenyMekhanik
Copy link

To use transactions over iproto streams you should set 'memtx_use_mvcc_engine = true' on remote server or use vinyl engine.
Simple example of using iproto streams:

conn = net_box.connect("remote server address")
stream = conn:new_stream()
conn_space = conn.space."space_name"
stream_space = stream.space."space_name"
stream:begin()
stream_space :replace({1})
stream_space :select({}) -- [1]
conn_space:select({}) --[] empty transaction was not commited
-- On remote server select is also empty
stream:commit()
conn_space:select({}) -- [1]

More examples in test/box/net.box_iproto_transactions_over_streams.test.lua


**IPROTO_STREAM_ID** = 0x0a.
An unsigned number that should be unique in every stream.
In requests IPROTO_STREAM_ID is optional and is only useful for ensuring that requests within transactions are done in separate groups.

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, in my second commit, I mentioned the first, with approximately the words that you suggested.


**IPROTO_COMMIT** = 0x0f.

This is for starting a transaction.

This comment was marked as resolved.

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, fixed.

"insert for stream #1", "insert for stream #2", "delete for stream #2", "delete
for stream #1", and then a "commit for stream #1" which the server instance will
interpret as a request to handle only the two inserts for stream #1.

This comment was marked as resolved.

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 see that my interleaving example was bad. I changed it so that it mentions only the requests, and avoided predicting what the server will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

@EvgenyMekhanik is the new version correct? Can we merge & publish this PR?


Thus there are now multiple ways to do transactions:
with net_box and stream:begin() and stream:commit() or stream:rollback()
which cause IPROTO_BEGIN and IPROTO_COMMIT or IPROTO_ROLLBACK with

Choose a reason for hiding this comment

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

IPROTO_BEGIN used to start transaction, IRPOTO_COMMIT to commit transaction and IPROTO_ROLLBACK used to rollback transaction.
There are multiple ways to start transaction:

  1. stream:begin()
  2. stream:call('box.begin')
  3. stream:eval('box.begin()')
  4. stream:execute('START TRANSACTION') -- sql syntax
    There are multiple ways to commit transactions:
  5. stream:commit()
  6. stream:call('box.commit')
  7. stream:eval('box.commit()')
  8. stream:execute('COMMIT')
    There are multiple ways to rollback transactions:
  9. stream:rollback()
  10. stream:call('box.rollback')
  11. stream:eval('box.rollback()')
  12. stream:execute('ROLLBACK')

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, but we won't need to mention all possibilities in the iproto description.

Copy link

@EvgenyMekhanik EvgenyMekhanik Nov 9, 2021

Choose a reason for hiding this comment

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

Here I would write:
"Thus there are now multiple ways to do transactions with net_box:
stream:begin() and stream:commit() or stream:rollback()
which cause IPROTO_BEGIN and IPROTO_COMMIT or IPROTO_ROLLBACK with ..."

But it's up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current sentence has clauses delimited with semicolons
"with net_box and ....;"
"with box.begin() and ...;"
"with SQL and ...".
Changing the initial words to "with net_box:" would make
it appear that the whole sentence is about net_box, but
only the first clause is about net_box.
So I will leave this unchanged, but thanks for the suggestion.

@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 November 5, 2021 18:43 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 November 5, 2021 18:43 Inactive
@pgulutzan
Copy link
Contributor Author

@EvgenyMekhanik, @NickVolynkin: After I did the first commit for this issue, Kseniia Antonova on October 21 with commit 9f10fa0 added a new section for the streams feature, https://www.tarantool.io/en/doc/latest/book/box/stream/#term-interactive-transaction Streams and interactive transactions. It is better than my too-short comments. So I added a :ref: to that section, which will be valid after this second commit is merged.

My comment at the start, about ER_TRANSACTION_YIELD, is solved -- as Evgeny Mekhanik explained, I must use vinyl or memtx/mvcc. But, assuming that others might make the same mistake that I made, I added a short phrase that indicates that is necessary.

Incidentally, when testing, I saw this message:
[ALSOFT] (EE) Failed to set real-time priority for thread: Operation not permitted (1)
I have not noticed this message before. But I assumed that it has nothing to do with streams, so I ignored it.

IPROTO_COMMIT or IPROTO_ROLLBACK.
Each request must contain the same IPROTO_STREAM_ID value.
With streaming there is no need to add
:ref:`IPROTO_FLAGS <box_protocol-flags>` + IPROTO_FLAG_COMMIT
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:ref:`IPROTO_FLAGS <box_protocol-flags>` + IPROTO_FLAG_COMMIT
:ref:`IPROTO_FLAGS <box_protocol-flags>` and IPROTO_FLAG_COMMIT

@pgulutzan Just to be clear, does + mean "and" here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One could say "and", but I prefer "+" and have used it elsewhere in the section.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pgulutzan the plus sign is reserved for the algebraic operation of addition and can be understood as such. We don't want to risk confusing our readers. Please use "and", "or", and other suitable words.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickVolynkin: See my third commit, as ordered.

@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 November 8, 2021 13:21 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 November 8, 2021 13:21 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 November 8, 2021 15:39 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 November 8, 2021 15:39 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 December 6, 2021 11:38 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 December 6, 2021 11:39 Inactive
Comment on lines +1104 to +1105
``34 00 = IPROTO_BIND_COUNT and MP_UINT = 0`` (there are no parameters to bind), |br|
``33 90 = IPROTO_BIND_METADATA and MP_ARRAY, size 0`` (there are no parameters to bind).
Copy link
Contributor

@patiencedaur patiencedaur Dec 6, 2021

Choose a reason for hiding this comment

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

Do you mean that byte 34 is IPROTO_BIND_COUNT and byte 00 is MP_UINT? Which one of them equals zero — only MP_UINT or both?

It is not clear how the equals sign works in this case, what parts are code and what are not. Same comment for strings 1663–64. Our translation team has trouble working with these expressions.

Our documentation guidelines provide some information on how to write about code, what is code and what is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the start we say IPROTO_BIND_COUNT is a value i.e. 34
and MP_UINT means unsigned integer i.e. MP_UINT is a type.
At the end we have an illustration:
34 IPROTO_BIND_COUNT
00 MP_INT = 0 = number of parameters to bind
33 IPROTO_BIND_METADATA
90 MP_ARRAY, size 0 = there are no parameters to bind
Incidentally on 2020-08-16 I wrote to E. Shebunyaeva suggesting I could
change occurrences of MP_INT to MP_UINT, but there was no decision.

@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 December 8, 2021 10:29 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 December 8, 2021 10:29 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 December 13, 2021 09:19 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 December 13, 2021 09:19 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 December 13, 2021 09:35 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 December 13, 2021 09:36 Inactive
@github-actions github-actions bot temporarily deployed to translation-pgulutzan-gh-2306 December 13, 2021 09:45 Inactive
@github-actions github-actions bot temporarily deployed to branch-pgulutzan-gh-2306 December 13, 2021 09:46 Inactive
@patiencedaur patiencedaur merged commit 67f26d6 into latest Dec 13, 2021
@patiencedaur patiencedaur deleted the pgulutzan-gh-2306 branch December 13, 2021 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants