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

Use DDP connection specified when creating a new JobCollection #84

Closed
mart-jansink opened this issue Jun 11, 2015 · 6 comments
Closed
Labels

Comments

@mart-jansink
Copy link

It may be a bit complicated, but I'm trying to create a three-tier program, consisting of three separate Meteor applications:

  • Up-top is the main server application that creates and controls JobCollection A.
  • In the middle is a application that both provides a worker for jobs from JobCollection A as well creates and controls JobCollection B (the worker actually creates multiple jobs in JobCollection B for each job from JobCollection A that it processes).
  • At the bottom is a third application that simply provides a worker for JobCollection B.

The middle application connects to the top one through DDP, likewise for the bottom and middle application. I'm running into an issue with the middle application: as Job.setDDP globally sets the DDP collection that should be used when calling things like job.save(), etc. I have to switch it between the DDP connection to the top application and the middle application's own DDP server.

Wouldn't it be more logical that this setting configurable per JobCollection? More specific, wouldn't it be more logical to use the connection specified in var myJobs = JobCollection('myJobQueue', {connection:TopApplication}); as we already specify which JobCollection should be use when calling var job = new Job(myJobs, ...);.

Would there be any downsides to this approach or was it a deliberate choice to use one global setting for the DDP connection? If not, I wouldn't mind creating a PR for this, although I don't usually work in Coffeescript.

@vsivsi
Copy link
Owner

vsivsi commented Jun 11, 2015

Hi, Thanks for reporting. This sounds from your description like a bug, but I'll need to reproduce what you're seeing to really dig into the observed vs expected behavior. Any chance you can produce a (relatively) simplified reproduction case that demonstrates the issue? As you said, this is a complicated scenario with three servers, etc. So any work you can do to create a simplified reproduction will help me to solve this issue more quickly. Thanks!

@vsivsi vsivsi added the bug label Jun 11, 2015
@mart-jansink
Copy link
Author

Luckily I'm still in the prototyping phase, so this was relatively easy, see https://github.com/mart-jansink/multi-tier-job-queue. Resetting the DDP server is done at https://github.com/mart-jansink/multi-tier-job-queue/blob/master/scotty/server/packages.js#L13. Thanks for your help :-)

@vsivsi
Copy link
Owner

vsivsi commented Jun 13, 2015

Hi, yes you've hit upon a case I didn't anticipate. The Job class stores the RPC method in a class variable that is then used by each job object to make such calls. In a pure node program this could be worked around by simply doing two requires:

var Job1 = require('meteor-job');
Job1.setDDP(ddpConnection1);

var Job2 = require('meteor-job');
Job2.setDDP(ddpConnection2);

var j1 = new Job1(jc1, 'type1', {});
var j2 = new Job2(jc2, 'type2', {});

But this approach obviously won't work for multiple Meteor servers; that's the case I failed to anticipate.

So I need to think about how to handle this without too much disruption to the 99% of devs who will never encounter this issue. I think it will be easy to solve if we use the existing polymorphism of the first parameter of the Job constructor:

var jc1 = new JobCollection("JC1");
var jc2 = new JobCollection("JC2", { connection: ddpConnection });

var job1 = new Job(jc1, 'type1', {});  // note: jc1 not "JC1"
var job2 = new Job(jc2, 'type2', {});  // note: jc2 not "JC2"

That way, the Job constructor can see the correct way to invoke the remote methods.

I'll prototype this up over the next few hours and see how it goes.

@vsivsi
Copy link
Owner

vsivsi commented Jun 13, 2015

Hi @mart-jansink, I've just committed a potential fix to this that works with my testing. It's on the dev branch here: https://github.com/vsivsi/meteor-job-collection/tree/dev

Please try this out in your setup and see if it resolves everything you are seeing... Let me know your results. If satisfactory, I will document this change and push a new release within 24 hours of hearing back from you.

If you haven't done package development before, you can use the dev branch code by:

# Create a packages directory in your app if one doesn't already exist
mkdir packages
cd packages
# Clone the repo and switch to the dev branch
git clone --recursive https://github.com/vsivsi/meteor-job-collection.git job-collection
cd job-collection
git checkout dev
cd ../..
# Now edit your ./meteor/versions file so that the vsivsi:job-collection line 
# requests version 1.1.4.  Your app should now use the dev version

@mart-jansink
Copy link
Author

Yes, this fixes the issue. Thanks for your swift response!

@vsivsi
Copy link
Owner

vsivsi commented Jun 15, 2015

Great. Just published in version 1.1.4 Thanks again for reporting this issue.

@vsivsi vsivsi closed this as completed Jun 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants