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.

Sign up for GitHub

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

Jump to bottom

JobManager: refactor, database imp, clustered imp #501

Merged
merged 2 commits into from
Mar 1, 2017
Merged

JobManager: refactor, database imp, clustered imp #501

merged 2 commits into from
Mar 1, 2017

Conversation

NielsCharlier
Copy link
Contributor

No description provided.

pvalsecc
pvalsecc reviewed Feb 24, 2017
View reviewed changes
Copy link
Contributor

@pvalsecc pvalsecc left a comment

Choose a reason for hiding this comment

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

It's missing some general documentation on how to enable the DB mode. Especially the CREATE TABLE you need for all that to work.

core/src/main/resources/mapfish-spring-application-context.xml Outdated
@@ -41,7 +42,8 @@
<property name="timeToKeepAfterAccessInMinutes" value="30" />
</bean>

<bean id="jobManager" class="org.mapfish.print.servlet.job.ThreadPoolJobManager">
<bean id="jobQueue" class="org.mapfish.print.servlet.job.impl.RegistryJobQueue"></bean>
Copy link
Contributor

Choose a reason for hiding this comment

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

weird indentation.

...rc/test/resources/org/mapfish/print/servlet/mapfish-spring-application-context-clustered.xml
<property name="properties">
<props>
<prop key="jobManager.clustered">true</prop>
<prop key="jobManager.oldFileCleanUp">false</prop>
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find where you delete old jobs in hibernate mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that was still missing, I have added it now.

@NielsCharlier
Copy link
Contributor Author

No need for a create table, that happens automatically. An empty database is sufficient

@pvalsecc
Copy link
Contributor

Looking good. There is just one thing that bothers me...
The JobManager doesn't get the jobs from the DB, but from it's in memory queue. Or did I miss something?

Extract from the spec:

  • The MFP instance adds this job to a job store (e.g. a Postgres database) that is accessible to all MFP instances.
  • The MFP instances keep polling the job store for new jobs.
  • When a MFP instance takes a job, this job is marked as running in the job store (SELECT ... FOR UPDATE).

@NielsCharlier
Copy link
Contributor Author

No, it's not from a memory queue.

  • "The MFP instance adds this job to a job store (e.g. a Postgres database) that is accessible to all MFP instances"

https://github.com/NielsCharlier/mapfish-print/blob/b850d0a425f5428688523bc03ebad016b83a4b7d/core/src/main/java/org/mapfish/print/servlet/job/impl/ThreadPoolJobManager.java#L312
(added to the database)

https://github.com/NielsCharlier/mapfish-print/blob/b850d0a425f5428688523bc03ebad016b83a4b7d/core/src/main/java/org/mapfish/print/servlet/job/impl/ThreadPoolJobManager.java#L334
(not immediately run)

https://github.com/mapfish/mapfish-print/pull/501/files#diff-7d47a534e424a7a068b45248cdaa4b78R175 (Lock = select for update)

pvalsecc
pvalsecc approved these changes Feb 27, 2017
View reviewed changes
Copy link
Contributor

@pvalsecc pvalsecc left a comment

Choose a reason for hiding this comment

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

OK, I've missed all the part about polling. LGTM!
Thanks

@pvalsecc pvalsecc merged commit 86b4a86 into mapfish:master Mar 1, 2017
@NielsCharlier NielsCharlier deleted the jobman branch May 2, 2017 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pvalsecc pvalsecc pvalsecc approved these changes

Assignees
No one assigned
Labels
no-changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NielsCharlier @pvalsecc @sbrunner