Skip to content

internal: master conn check for get space utils #336

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 20, 2023
Merged

internal: master conn check for get space utils #336

merged 2 commits into from
Jan 20, 2023

Conversation

GRISHNOV
Copy link
Contributor

@GRISHNOV GRISHNOV commented Dec 28, 2022 •
edited
Loading

Added validation of the connection to the utils.get_space method before receiving the space through the connection.

Error before patch:

...
str: 'SelectError: ...app/.rocks/share/tarantool/crud/common/sharding/init.lua:183:
    ...repro/myapp/.rocks/share/tarantool/crud/common/utils.lua:100: attempt to index
    field ''space'' (a nil value)'
...

Now:

...
str: 'SelectError: An error occurred during the operation: "GetSpaceError: The connection
    to the master of replicaset 548fb6ff-d686-4298-ad17-7f81fa19588c is not valid:
    connect, called on fd 40, aka 127.0.0.1:58334: Connection refused"'
...

Closes #331

Totktonada
Totktonada reviewed Dec 28, 2022
View reviewed changes
crud/common/utils.lua Outdated
Comment on lines 101 to 121
if replicaset.master.conn.error ~= nil then
local error_msg = string.format('The connection to the master is not valid: %s',
replicaset.master.conn.error)
error(error_msg)
end
Copy link
Member

Choose a reason for hiding this comment

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

According to #95 it is also possible that replicaset.master is nil. The issue is pretty old, but maybe it worth to re-check it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! So far I've implemented a simple check replicaset.master is not nil, but it is not entirely clear to me where the timeout for this check is set from

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch from f3ab53a to d85e82b Compare January 10, 2023 08:51
DifferentialOrange
DifferentialOrange reviewed Jan 10, 2023
View reviewed changes
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch from d85e82b to 8bfa3b5 Compare January 16, 2023 09:22
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch 3 times, most recently from e9fecfe to 9538e49 Compare January 17, 2023 11:36
@GRISHNOV
Copy link
Contributor Author

Also, while working on the task, I noticed that this line of the code triggers an error:

myapp.router> crud.insert('customer', {1,100,'Mike'})
---
- error: '...epro/myapp/.rocks/share/tarantool/crud/common/schema.lua:40: calling
    ''status'' on bad self (fiber expected, got table)'
...

AFAIU, it should be f:status(), not fiber:status()

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch from 9538e49 to 3789c9d Compare January 17, 2023 12:38
@DifferentialOrange
Copy link
Member

Also, while working on the task, I noticed that this line of the code triggers an error:

myapp.router> crud.insert('customer', {1,100,'Mike'})
---
- error: '...epro/myapp/.rocks/share/tarantool/crud/common/schema.lua:40: calling
    ''status'' on bad self (fiber expected, got table)'
...

AFAIU, it should be f:status(), not fiber:status()

It would be nice to fix it in a separate commit, especially if you've got a repro (should be ok even without one, if this is complicated).

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch 2 times, most recently from 1135bd2 to b36cf6c Compare January 18, 2023 09:37
@GRISHNOV GRISHNOV marked this pull request as ready for review January 18, 2023 09:39
@GRISHNOV
Copy link
Contributor Author

Thanks for the feedback! Now the PR is prepared for the #331

DifferentialOrange
DifferentialOrange reviewed Jan 18, 2023
View reviewed changes
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you for your updates! There are some older questions that are still unresolved: I've either never got the final answer or have missed it -- please, respond to them here so all of our decisions would be preserved here.

@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch 3 times, most recently from f6f790b to 8fe5f81 Compare January 19, 2023 08:56
DifferentialOrange
DifferentialOrange reviewed Jan 19, 2023
View reviewed changes
DifferentialOrange
DifferentialOrange reviewed Jan 19, 2023
View reviewed changes
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch from 8fe5f81 to e437c5a Compare January 19, 2023 09:19
DifferentialOrange
DifferentialOrange reviewed Jan 19, 2023
View reviewed changes
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch from e437c5a to 2faf18c Compare January 19, 2023 09:55
DifferentialOrange
DifferentialOrange approved these changes Jan 19, 2023
View reviewed changes
Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you for the updates! Let's change a couple of cosmetic things and merge it.

@GRISHNOV
internal: master presence check for get space
9bf65ec 
Added validation of the master presence in replicaset and the
master connection to the `utils.get_space` method before
receiving the space from the connection.

Closes #331
@GRISHNOV
bugfix: fiber cancel on schema reload timeout
906a2d3 
Fixed fiber cancel on schema reload timeout.
Before that, an error occurred in timeout case with description:
`calling 'status' on bad self (fiber expected, got table)`.
To reproduce the error before this fix, it is enough to
checkout to the one commit back and perform the action
described in #331.
@GRISHNOV GRISHNOV force-pushed the igrishnov/gh-331-master-conn-check-for-get-space-utils branch from 2faf18c to 906a2d3 Compare January 19, 2023 15:09
@DifferentialOrange DifferentialOrange merged commit 73bf5bf into master Jan 20, 2023
@DifferentialOrange DifferentialOrange deleted the igrishnov/gh-331-master-conn-check-for-get-space-utils branch January 20, 2023 07:34
@GRISHNOV GRISHNOV mentioned this pull request Feb 2, 2023
Merged
1 task
DifferentialOrange added a commit that referenced this pull request Feb 2, 2023
@DifferentialOrange
release: 1.0.0
93f834d 
Overview

  This release introduces a breaking change with removing a deprecated
  feature: `crud.len(space_id)`.

  This release also introduces a Cartridge clusterwide config to setup
  `crud.cfg`.

Breaking changes

  You cannot use space id as a space identifier in `crud.len` anymore.
  Use space name instead.

New features

  * Timeout condition for the validation of master presence in 
    replicaset and for the master connection (#95).
  * Cartridge clusterwide configuration for `crud.cfg` (#332).

Changes

  * Forbid using space id in `crud.len` (#255).

Fixes

  * Add validation of the master presence in replicaset and the 
    master connection to the `utils.get_space` method before 
    receiving the space from the connection (#331).
  * Fix fiber cancel on schema reload timeout in `call_reload_schema`
    (PR #336).
DifferentialOrange added a commit that referenced this pull request Feb 2, 2023
@DifferentialOrange
release: 1.0.0
bd39ec8 
Overview

  This release introduces a breaking change with removing a deprecated
  feature: `crud.len(space_id)`.

  This release also introduces a Cartridge clusterwide config to setup
  `crud.cfg`.

Breaking changes

  You cannot use space id as a space identifier in `crud.len` anymore.
  Use space name instead.

New features

  * Timeout condition for the validation of master presence in 
    replicaset and for the master connection (#95).
  * Cartridge clusterwide configuration for `crud.cfg` (#332).

Changes

  * Forbid using space id in `crud.len` (#255).

Fixes

  * Add validation of the master presence in replicaset and the 
    master connection to the `utils.get_space` method before 
    receiving the space from the connection (#331).
  * Fix fiber cancel on schema reload timeout in `call_reload_schema`
    (PR #336).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Totktonada Totktonada Totktonada left review comments

@DifferentialOrange DifferentialOrange DifferentialOrange approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unclear error if no leader
3 participants
@GRISHNOV @DifferentialOrange @Totktonada