Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce concurrency controls #38
Introduce concurrency controls #38
Changes from all commits
e1c7884
f405340
4ce223a
6050088
5c14277
e217ae0
873760d
d0eeed2
9fa0aa2
7ab243d
04dd571
7814338
016674b
f22a46c
ede2bfb
8484ca8
91256ee
9f80e5f
8993dd9
cfaf02f
6e14ea0
7c3007a
363ae88
fbef301
427482e
93eb7f8
ee5fcc6
feef30b
9d0da03
49f7d13
77b67b0
8a5de86
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your consideration: the semaphore static interface where it receives a job and then pass several jobs' attributes around feels like it could benefit from an extracting a method object. I understand why you did this, since it's a special class that acts on group of records, not a regular active record object. Still you may consider to use an additional inner class with a name like
Control
orProxy
that exposes the interface you want for a given job. I haven't tested this, but it could look something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jorgemanrubia! I'll think about it and see how it could look like. I agree with you, passing the job attributes around doesn't look so nice, and you're spot on about why I did it this way.
Thanks so much for your suggestion, I'll play with it 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorgemanrubia, I took a stab at it in 61fec0d, what do you think? I think I like it! I chose to keep the methods
Semaphore.signal(job)
andSemaphore.wait(job)
, but encapsulate the rest. I like to use the static interface from outside, although I don't feel strongly.