If you are unsure about the correct group, please do not leave the issue without a group label, and refer to
GitLab's shared responsibility functionality guidelines
for more information on how to triage this kind of issue.
This issue was automatically tagged with the label groupfoundations by TanukiStan, a machine learning classification model, with a probability of 0.21.
If this label is incorrect, please tag this issue with the correct group label as well as automation:ml wrong to help TanukiStan learn from its mistakes.
Authors who do not have permission to update labels can use the @gitlab-bot label ~"group::<correct group name>" command, or leave the issue to be triaged by group leaders initially assigned by TanukiStan.
This message was generated automatically.
You're welcome to improve it.
Is the ownership for buffered counters still with the devopsverify?
That said, this issue does not necessarily require a "fix". If the domain expert deems that the transaction scope is small enough and there are no clear workarounds, an MR to update the comments in lib/gitlab/counters/buffered_counter.rb would be fine.
@iamricecake I've updated the description hope that helps! If it is not possible to move the exclusive lease out of the transaction, we would need to assess the scope of the transaction, what locks does it acquire and the impact if the transaction were to be idle.
@schin1 trying to understand, please correct me if I am wrong, but looking at the said commit_increment! method:
defcommit_increment!with_exclusive_leasedoflush_amount=amount_to_be_flushednextifflush_amount==0counter_record.transactiondo# Exclusive lease is obtained within `counter_record.transaction` which could lead to idle transactions# if exclusive lease Redis I/O is slow. We skip the transaction check for now.# See issue: https://gitlab.com/gitlab-org/gitlab/-/issues/441530Gitlab::ExclusiveLease.skipping_transaction_checkdocounter_record.update_counters_with_lease({attribute=>flush_amount})endremove_flushed_keyendcounter_record.execute_after_commit_callbacksendcounter_record.reset.read_attribute(attribute)end
And the said problem:
Problem: commit_increment! updates counters using an exclusive lease while in a counter_record.transaction
Isn't it the other way around? The transaction is opened only right after the lease has been obtained which happens once with_exclusive_lease is called?
Just a thought but perhaps we could update counters without lease if there is an exclusive lease already obtained in the outer scope?
@schin1 yes this is what I was thinking as well. But that will skip the detect_race_on_record which was intentionally added there to track race conditions.
If the current implementation isn't causing any problems, can we just add a comment on why we need the update_counters_with_lease and also why it is inside a transaction?
Reasons:
update_counters_with_lease internally uses detect_race_on_record to track potential race conditions when concurrently updating a counter.
We need the transaction so we can rollback the counter update if remove_flushed_key fails.
If the current implementation isn't causing any problems, can we just add a comment on why we need the update_counters_with_lease and also why it is inside a transaction?
@iamricecake@schin1@jocelynjane As far as I can tell, the lease doesn't actually do anything in the context of the FlushCounterIncrementsWorker: https://gitlab.com/gitlab-org/gitlab/-/issues/482785#note_2089198910. It just "tracks" when there was a conflict, but it doesn't handle it. Can we, for the use case of the FlushCounterIncrementsWorker rely solely on the outer lease that is already there?
I am not the right person to answer questions around the design of flushing increments and how this specific counter plays into that. Tagging @carolinesimpson and @rutshah as this now falls under grouppipeline execution.