Skip to content

Address feedback on previous postponed job PR #9187

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

Merged

Conversation

KJTsanaktsidis
Copy link
Contributor

This PR just has some minor fixes in response to feedback in #8949

  • Doc enhancements
  • Refactor some local variable names
  • Revert the changes to the tracepoint tests (they are unnescessary with the semantics of rb_postponed_job_preregister we finally settled on)
  • Added a test case to assert that multiple calls to preregister updates the stored data.

KJ Tsanaktsidis added 4 commits December 12, 2023 10:45
Just removes the unneeded `prereg_` prefix from a few local var names.
With the latest version of the postponed job patchset merged, we don't
actually need to go through the contortions of keeping the data in a
global variable; we can just update `data` with multiple calls to
rb_postponed_job_preregister.
We want to make sure that if preregister is called with different data,
that the postponed job table is updated.
@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/postponed_job_feedback branch from cb1c8bd to 683be71 Compare December 11, 2023 23:57
@KJTsanaktsidis
Copy link
Contributor Author

@ko1 if you're happy with this, are you able to merge it please? 🙏

@@ -651,6 +651,18 @@ The following APIs are updated.
Extension libraries which use this interface and built for older
versions need to rebuild with adding `init_int32` function.

* `rb_postponed_job` changes
Copy link
Member

Choose a reason for hiding this comment

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

Is it intentional? This file is for Ruby 3.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 Nope, this is a mistake - thank you for catching it, I will push up a fix now.

@KJTsanaktsidis KJTsanaktsidis force-pushed the ktsanaktsidis/postponed_job_feedback branch from 683be71 to af49814 Compare December 12, 2023 09:56
@KJTsanaktsidis KJTsanaktsidis merged commit a2994c3 into ruby:master Dec 13, 2023
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