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

Added hierarchical lock for applying data updates #745

Merged
merged 13 commits into from
Feb 2, 2025

Conversation

danyi1212
Copy link
Collaborator

Fixes Issue

Changes proposed

Check List (Check all the applicable boxes)

  • I sign off on contributing this submission to open-source
  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

@danyi1212 danyi1212 self-assigned this Jan 27, 2025
Copy link

netlify bot commented Jan 27, 2025

Deploy Preview for opal-docs canceled.

Name Link
🔨 Latest commit 1b73871
🔍 Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/679fb5546eb69b0008664bf5

Copy link
Contributor

@orweis orweis left a comment

Choose a reason for hiding this comment

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

This is a good effort but:

  1. You focused too much on restyling the code that it made reviewing very difficult
  2. You have little to no documentation / comments - (you actually removed comments) - on a super sensitive code like this - it's too risky
  3. and most crucial - the design breaks a core concurrency concept (queue for fetching) which would hurt the performance of this component dramatically (we actually discussed this in the design phase)

Copy link
Contributor

@orweis orweis left a comment

Choose a reason for hiding this comment

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

Few more comments

packages/opal-client/opal_client/data/updater.py Outdated Show resolved Hide resolved
packages/opal-client/opal_client/data/updater.py Outdated Show resolved Hide resolved
Copy link
Contributor

@orweis orweis left a comment

Choose a reason for hiding this comment

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

Great and important work.

Few last comments placed for final touches.

Celebration:
The Hierarchical Lock mechanism is very elegant

Lessons for next time:
Keep it more focused on the core task, and add comments.

@danyi1212 danyi1212 force-pushed the dan/per-11699-fix-race-for-opal-data-updates branch from 1352480 to 027d81c Compare February 2, 2025 16:51
Copy link
Contributor

@orweis orweis left a comment

Choose a reason for hiding this comment

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

Job well done.

@danyi1212 danyi1212 merged commit e6690be into master Feb 2, 2025
11 checks passed
@danyi1212 danyi1212 deleted the dan/per-11699-fix-race-for-opal-data-updates branch February 2, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants