Keep track of PID of process running job #1

roxelo · 2020-06-12T20:29:23Z

This PR is still a work in progress, but so far this is what has been done:

Retrieve PID of process running job
Updated 2 git commands
- Use -B instead of -b for creating new branch. (-B create the new branch only if the branch does not exists, otherwise it resets the branch
- Added -p to the git fetch --all command, since @arora-aman mentioned it was better

Next to implement:

Figure out the best way to keep track of the process PID so we can easily terminate processes and implement it

Closes sexxi-goose/sexxi-sync#6

src/lib/config.rs

src/lib/cmd.rs

arora-aman · 2020-06-12T20:52:34Z

-p all gets information about branches that were deleted from the remote. However that doesn't mean that it will delete the local tracking branches.

I know someone wrote a utility to make that happen as well, but I don't remeber what that is called lol.

…ef is requested

roxelo · 2020-06-15T23:29:07Z

This kind of overlaps with sexxi-goose/sexxi-sync#7. I am considering maybe combining both tasks into one PR.

src/lib/cmd.rs

src/lib/job.rs

roxelo · 2020-06-16T20:12:53Z

If this looks good for tracking PID, I will go ahead and merge. This PR does terminate a job if the bot is removed and then re-added, but I decided I will just make a different PR for the conditions that require terminating a job.

Azhng

Left some nits for code style. But one thing that confuses me was why are we passing around curr_job ? It seems like it is used to detect if a job is already running for a given head_ref. I would much prefer we always go to JobRegistry as the source of truth to fetch the job information. Though that might require some modification of the JobRegistry to make it fast.

src/lib/config.rs

src/lib/handler.rs

src/lib/job.rs

roxelo · 2020-06-16T20:49:15Z

As I mentioned on Discord, JobRegistry by itself doesn't track the information we need. We could pass around JobRegistry but we would still need to know what the uuid of the running job is. That implies that we would be passing that around as well. I guess we could approach solving this by turning JobRegistry into a class/trait rather than a type, but before I go ahead and make those changes it would be best to both agree on what JobRegistry should look like for what we want it to do.

roxelo · 2020-06-17T04:34:52Z

More testing to be performed tomorrow.
PID of running jobs are now being tracked in JobRegistry.
Conditions which should terminate a running jobs (sexxi-goose/sexxi-sync#5) will be added in a different PR for readability.

Azhng

Awesome work! Just few comments!

src/lib/config.rs

src/lib/job.rs

Azhng · 2020-06-17T04:55:23Z

src/lib/job.rs

+           have a deadlock. We want to update the JobDesc for the job with the PID of the
+           running process, but since we have the read lock, we can't acquire the write lock.
+
+           If we want to avoid having to request the read lock everytime we need it, we should


What about instead of a deep copy we just clone the Arc<RwLock<JobDesc>>, i know it's a bit boilerplate but we went through all these trouble just so we can avoid deep copy. I think current code works good so it's just my personal preference. Also implementing a macro can probably reduce the repetitive locking/unlocking.

@arora-aman thoughts ?

If we are doing a deep copy then we should probably do it earlier and write some other way of returning the status. But the way code is currently written it makes more sense to just clone the Arc instead of the cloning the underlying JobDesc

I am not sure if that really is needed.
Things to keep in mind, why make it more complicated right now when the job is only used for information so far, which should technically not be modified (except for PID, but let's ignore that for now).
Second, the JobRegistry is now being passed around as well as the running job uuid, so technically no need to pass the Arc<RwLock<JobDesc>> cause we can get it from the JobRegistry directly.

if u clone the Arc it allows for you to modify the Desc without need to hold lock over the registry. However I don't think it is really need in our case.

I'm not 100% faimilar with this codebase so I could be wrong, but start_build_job(job.clone(), ..) seems simpler, and would work.

hmm no, cloning the Arc still require the read lock

So I am not doing anything about this. Like the comment I left in the code stated, I don't think it is worth adding more concurrency complexity with the read lock when the data of JobDesc is not expected to change. A comment reflects this decision, if we want to change that we should be making a new issue.

src/lib/job.rs

src/lib/cmd.rs

arora-aman · 2020-06-17T05:53:14Z

src/lib/job.rs

+           have a deadlock. We want to update the JobDesc for the job with the PID of the
+           running process, but since we have the read lock, we can't acquire the write lock.
+
+           If we want to avoid having to request the read lock everytime we need it, we should


If we are doing a deep copy then we should probably do it earlier and write some other way of returning the status. But the way code is currently written it makes more sense to just clone the Arc instead of the cloning the underlying JobDesc

Azhng

Let's fix the locking issue in #2

Acquire job PID

e8d5866

roxelo added the wip work in progress label Jun 12, 2020

roxelo requested a review from Azhng June 12, 2020 20:29

Azhng reviewed Jun 12, 2020

View reviewed changes

src/lib/config.rs Outdated Show resolved Hide resolved

src/lib/cmd.rs Outdated Show resolved Hide resolved

src/lib/cmd.rs Outdated Show resolved Hide resolved

src/lib/cmd.rs Outdated Show resolved Hide resolved

src/lib/cmd.rs Outdated Show resolved Hide resolved

Fixing issues and killing processes when a new job on the same head_r…

826a4b6

…ef is requested

Azhng reviewed Jun 15, 2020

View reviewed changes

src/lib/cmd.rs Outdated Show resolved Hide resolved

src/lib/cmd.rs Outdated Show resolved Hide resolved

src/lib/job.rs Outdated Show resolved Hide resolved

Addressed comments left on latest commit

22e0562

roxelo removed the wip work in progress label Jun 16, 2020

Azhng requested changes Jun 16, 2020

View reviewed changes

src/lib/config.rs Outdated Show resolved Hide resolved

src/lib/handler.rs Outdated Show resolved Hide resolved

src/lib/handler.rs Outdated Show resolved Hide resolved

src/lib/job.rs Outdated Show resolved Hide resolved

roxelo added 2 commits June 16, 2020 19:30

Leverage JobRegistry to track running jobs

22f5b4c

update JobRegistry to use a hashmap instead

e5c80cf

Azhng reviewed Jun 17, 2020

View reviewed changes

arora-aman requested changes Jun 17, 2020

View reviewed changes

roxelo mentioned this pull request Jun 17, 2020

Use proper concurrency principles to access a running job JobDesc #2

Closed

Azhng approved these changes Jun 17, 2020

View reviewed changes

Addess comments

b726894

roxelo linked an issue Jun 17, 2020 that may be closed by this pull request

Use proper concurrency principles to access a running job JobDesc #2

Closed

roxelo removed a link to an issue Jun 17, 2020

Use proper concurrency principles to access a running job JobDesc #2

Closed

roxelo merged commit 5c17ee1 into master Jun 17, 2020

roxelo deleted the roxane/get-job-pid branch June 17, 2020 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep track of PID of process running job #1

Keep track of PID of process running job #1

roxelo commented Jun 12, 2020 •

edited

Loading

arora-aman commented Jun 12, 2020

roxelo commented Jun 15, 2020

roxelo commented Jun 16, 2020

Azhng left a comment •

edited

Loading

roxelo commented Jun 16, 2020 •

edited

Loading

roxelo commented Jun 17, 2020

Azhng left a comment

Azhng Jun 17, 2020

arora-aman Jun 17, 2020

roxelo Jun 17, 2020

arora-aman Jun 17, 2020 •

edited

Loading

arora-aman Jun 17, 2020

roxelo Jun 17, 2020

roxelo Jun 17, 2020 •

edited

Loading

arora-aman Jun 17, 2020

Azhng left a comment

Keep track of PID of process running job #1

Keep track of PID of process running job #1

Conversation

roxelo commented Jun 12, 2020 • edited Loading

arora-aman commented Jun 12, 2020

roxelo commented Jun 15, 2020

roxelo commented Jun 16, 2020

Azhng left a comment • edited Loading

Choose a reason for hiding this comment

roxelo commented Jun 16, 2020 • edited Loading

roxelo commented Jun 17, 2020

Azhng left a comment

Choose a reason for hiding this comment

Azhng Jun 17, 2020

Choose a reason for hiding this comment

arora-aman Jun 17, 2020

Choose a reason for hiding this comment

roxelo Jun 17, 2020

Choose a reason for hiding this comment

arora-aman Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

arora-aman Jun 17, 2020

Choose a reason for hiding this comment

roxelo Jun 17, 2020

Choose a reason for hiding this comment

roxelo Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

arora-aman Jun 17, 2020

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

roxelo commented Jun 12, 2020 •

edited

Loading

Azhng left a comment •

edited

Loading

roxelo commented Jun 16, 2020 •

edited

Loading

arora-aman Jun 17, 2020 •

edited

Loading

roxelo Jun 17, 2020 •

edited

Loading