ajvb / kala

Modern Job Scheduler
MIT License
2.13k stars 188 forks source link

Remote support #139

Closed levrado closed 7 years ago

levrado commented 7 years ago

Added support for jobType as discussed in #73 with a remote job type

basically it adds the following support:

if user passed job_type: 1 in the create job json, it allows her/him to specify remote_properties:

all params have default values and all except for url are optional

levrado commented 7 years ago

@ajvb Hi, it seems PR failed for some failure communicating with coveralls and not from my changes, could you please check and rerun? the message: Bad response status from coveralls: 422 - {"message":"Couldn't find a repository matching this job.","error":true}

ajvb commented 7 years ago

@levrado Thank you for the PR! This is looking great so far.

  1. First off, could we make sure to add comments to some of the new types and functions? As well, can we add some tests that hit a mock http server?

  2. In regards to the coveralls, lets just rip it out in this PR. It has caused me tons of problems, and its easier to just run a coverage test locally.

You will want to delete everything from the test section except these lines: https://github.com/ajvb/kala/blob/master/circle.yml#L26-L28

levrado commented 7 years ago

@ajvb added tests, comments and removed coveralls

levrado commented 7 years ago

@ajvb can you please update me the status of the review? tnx

ajvb commented 7 years ago

hey @levrado, sorry for the delay. I will review today.

ajvb commented 7 years ago

@elimisteve Would love a 👍 from you if you have the time :)

levrado commented 7 years ago

@ajvb @elimisteve done changing according to pr comments

circleci is complaining on a data race that i can't seem to replicate on my computer (although i did found a problem in TestOneOffJobs in job_test which i fixed by moving the locking 2 lines up)

could you please have a look and tell me if the error happening with the current master on circleci?

elimisteve commented 7 years ago

@levrado Are there any tests that share state? I don't see anything being run concurrently at all, so I'm not sure how and where the race could be coming from.

elimisteve commented 7 years ago

@levrado Instead of calling every job mock_job, could you number them, or include a high-precision timestamp so that we can tell which mock job is involved in the race?

I don't see much concurrent happening unless the job is unscheduled, in which case it runs immediately: https://github.com/ajvb/kala/blob/cd5ec293a21d527aa6a745df62f6c1e8f9a3fbcb/job/job.go#L163

levrado commented 7 years ago

@elimisteve the tests that I wrote are with "mock_remote_job" in the job name, those are not my tests that are failing

also it's not happening on my computer (running the same command as circleci) and didn't failed on circleci 2 weeks ago as well.

is it possible to rerun the master branch on circleci? to eliminate that those are my changes causing it

elimisteve commented 7 years ago

Found it: shParser is being used concurrently, which isn't safe: https://github.com/ajvb/kala/commit/a0c7cc91c21e24d248b0b646b9d04eb497b559dd#commitcomment-20631329

levrado commented 7 years ago

@elimisteve good catch, weird that it passed ok till now for you guys

changed it to not be reused but be created on every new command run

elimisteve commented 7 years ago

@ajvb @levrado I'm not seeing the entire big picture, but in the small at least, LGTM :-) :+1: