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.

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

fix(request-queue): Update request queue locking docs #1322

Merged
merged 7 commits into from
Dec 10, 2024

Conversation

drobnikj
Copy link
Member

@drobnikj drobnikj commented Dec 6, 2024

I made some changes in docs regarding the API changes.

  • stress out that locking works on the client level and the same as on the run level.
  • update code example by separating code into two actors using code tabs

@drobnikj drobnikj requested a review from TC-MO as a code owner December 6, 2024 13:12
@github-actions github-actions bot added this to the 104th sprint - Platform team milestone Dec 6, 2024
@github-actions github-actions bot added the t-platform Issues with this label are in the ownership of the platform team. label Dec 6, 2024
@drobnikj drobnikj requested a review from fnesveda December 6, 2024 13:14
Copy link
Member

@fnesveda fnesveda left a comment

Choose a reason for hiding this comment

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

Looks good, but the variable naming is a bit confusing, could you improve it?

Comment on lines 475 to 476
const theFirstRequestLockedByClientOne = processingRequestsClientOne.items[0];
const requestLockedByClientOne = await requestQueueClientOne.getRequest(
Copy link
Member

Choose a reason for hiding this comment

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

The naming of these two variables is pretty confusing, I don't really know what's the difference between theFirstRequestLockedByClientOne and requestLockedByClientOne, especially by name. Could you come up with some better names?

const theFirstRequestLockedByClientOne = processingRequestsClientOne.items[0];
const requestLockedByClientOne = await requestQueueClientOne.getRequest(
theFirstRequestLockedByClientOne.id,
const wasTheClientTwoLockedSameRequest = !!processingRequestsClientTwo.items.find(
Copy link
Member

Choose a reason for hiding this comment

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

Here I'm also not sure what the varaible means, could you rename it, or explain better in the comment?

@drobnikj drobnikj changed the title fix(request-queue): Update request qeue locking docs fix(request-queue): Update request queue locking docs Dec 9, 2024
sources/platform/storage/request_queue.md Outdated Show resolved Hide resolved
sources/platform/storage/request_queue.md Outdated Show resolved Hide resolved
Co-authored-by: Michał Olender <[email protected]>
@drobnikj drobnikj requested a review from TC-MO December 10, 2024 09:37
@drobnikj drobnikj merged commit 7951a66 into master Dec 10, 2024
8 checks passed
@drobnikj drobnikj deleted the feat/improve_rq_locks_docs branch December 10, 2024 09:41
@fnesveda fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-platform Issues with this label are in the ownership of the platform team. validated Issues that are resolved and their solutions fulfill the acceptance criteria.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants