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.

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

chore: adding row lock with behavior #1081

Closed
wants to merge 1 commit into from
Closed

chore: adding row lock with behavior #1081

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion modules/ingestor/src/graph/organization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use sea_orm::{ActiveModelTrait, ColumnTrait, ConnectionTrait, EntityTrait, QueryFilter, Set};
use sea_orm::{
ActiveModelTrait, ColumnTrait, ConnectionTrait, EntityTrait, QueryFilter, QuerySelect, Set,
};
use sea_query::{LockBehavior, LockType};
use std::fmt::Debug;
use tracing::instrument;
use trustify_entity::organization;
Expand Down Expand Up @@ -62,6 +65,7 @@ impl Graph {
) -> Result<Option<OrganizationContext>, Error> {
Ok(organization::Entity::find()
.filter(organization::Column::Name.eq(name.into()))
.lock_with_behavior(LockType::Update, LockBehavior::SkipLocked)
.one(connection)
Copy link
Contributor

Choose a reason for hiding this comment

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

We are getting an organization, we do we need to lock that row?

Also, why would we want to skip that row in a case it's locked?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from the docs:
With SKIP LOCKED, any selected rows that cannot be immediately locked are skipped. Skipping locked rows provides an inconsistent view of the data, so this is not suitable for general purpose work, but can be used to avoid lock contention with multiple consumers accessing a queue-like table.

my main motivation for this is but can be used to avoid lock contention with multiple consumers
to solve this update problem

I sent the PR mostly to share the app behavior with the team, I'm not fan of adding db locks

Copy link
Contributor

Choose a reason for hiding this comment

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

So the result would be that anyone getting the organization would lock it (until the transaction expires) and that would make other, concurrent request skip that record?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I understand of this is that it blocks the select row and ignores the already blocked, by other concurrent calls.
I think is the same what you wrote.
Again I'm not happy with this also I'm using a small set of files for the upload test via ui.

Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, then I don't think it would be a good change.

Copy link
Collaborator Author

@helio-frota helio-frota Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah not a problem 👍 the PR is more for me to have second thought thanks

.await?
.map(|organization| OrganizationContext::new(self, organization)))
Expand Down
Loading