-
Notifications
You must be signed in to change notification settings - Fork 484
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
Proposal: make persistent EJB timers non-persistent or not rely on database #5345
Comments
@poikilotherm At present, we are not benefiting from the fact that the timers are stored in the database, at all. It's kind of the other way around: you can say that we go to the extra trouble of making sure that our timers are NOT persisted. In a multi-node cluster, we specifically try to run all the timers on just one of the nodes (for various reasons); AND we clear and re-create all the timers on startup. So yes, it will be cleaner, not to bother saving them in the database in the first place. (we actually have tried something similar, in our own production cluster at Harvard a while ago; I can't remember now why we chose not to make it the default solution in the distribution...) I may have questions about some specifics of the PR (#5371) though; I'll be in touch. |
Hey @landreev, could you give my comments in 81fbffe a look and discuss this with others? I am uncertain if this refactoring is a chunk to big to fit inside this PR. (Happy to do the necessary code work). And while you are in there, you could take a look at a7cc7b8 which puts the first (but not all) Thx! |
@poikilotherm I'm looking into it now (was out on Friday).
|
* This removes the old timer handling completly. No persistent timers present anymore. * Removes timer handling from the Harvester Client Admin UI backing. * This cannot be unit tested as stated in the Javadocs and accepted by @landreev. * This might need refactoring when problems arise from the simple execution approach used for now. Prior to this commit, harvest clients ran on independent timers and thus on independent threads. Now they are executed sequentially, which might lead to problems when harvests last longer and more clients want to run an hour later.
Hi @landreev feel free to look into my recent additions to #5371 😄 I added the necessary bits to Gave this a shot in VM, harvesting from Harvard Dataverse. It works, but we might run into trouble with such large harvests. Nothing would be really "lost", but it would take a few days to get this loads of data inside a new Dataverse installation. Should I add presumptious support for this (e.g. using JBatch or similar)? |
@poikilotherm hi! Two things:
|
IMHO this is still a WIP as long as @landreev does not express his satisfaction with this. Especially the possible design drawback mentioned above (large harvests causing delay for others) needs a decision from him... 😉 (Merging done.) |
The issue with long harvesting jobs - it's not really anything new/being introduced in this PR, correct? I'm currently looking at the overall code in the PR; it looks good overall, I just need to review more carefully the part where the scheduled client jobs are started. |
(I'm going to test the branch and run some harvests too) |
It is not really new, no. Before my code changes, the harvests are executed in parallel, as each harvester has its own timer going off and triggering the harvest a configured. The code changes lead to a serialisation of harvests, which might be suboptimal with bigger harvest in mind. The "A while" is a very unsharp term. When I harvested Harvard Dataverse, many thousand datasets where collected and it took a few hours until I aborted unfinished. OAI seems pretty slow... There are proper solutions around this like starting a background batch job from the scheduled code, but this might be presumptios optimization. |
I thought about this/discussed it a bit w/others, and I do think the serialization of harvests is a serious problem. I don't think we want to put all the harvests on hold, for hours (or days, potentially) while a large collection is being populated. And I'm not sure getting rid of the timer entries in the database is by itself worth a new limitation like this. I was actually hoping that this new code would be starting the individual jobs asynchronously (you mentioned this as a solution in your last comment). So we would be back to running harvests in parallel... Why not just do that - just use a method with the @asynchronous annotation? We will not be able to use the @lock(WRITE) annotation then; but that just means we will have to rely on the current system of the database locks on individual harvestingClients. Seems like a practical enough solution, I think. (?) |
Hey guys, I haven't forgotten this issue, I have been busy with other stuff. Sry for the delay. I will look into using proper JSR 352 (JBatch) handling for this. IMHO using Will keep in mind multiple glassfish servers as pointed out by @pdurbin on IRC. Using ShedLock might be a valid option. This will take a while, as I am on vacation for the two weeks ahead. |
Hi Oliver, there's definitely no rush, working on this issue. We are fixing something that's not really broken, after all... |
With the move to Payara done since 5.0, we could stop relying on a EJB timers PostgreSQL connection two ways (showstopper for #6819 and #7048).
Maybe other components like File PIDs, Draft PIDs etc benefit from this, too. |
This is related to #5292 and a story of that epic.
As
DataverseTimerServiceBean.java
has been mostly written and maintained by @landreev, this most certainly is for him...Context
Currently, in
glassfish-setup.sh
you need to setup a database connection for storage of persistent EJB timers.Reusing the database for Dataverse for EJB timers is alright, but it is not easy to depict when heading for containers. It is much easier for container approaches to use application scoped JDBC connections, but those seem not to be reusable for EJB timers.
There seemed to be some issues in the past, too. #3672, #3789 and more.
Proposal
Just remove the persistent nature of timers.
When persistance is needed in a multi node setup in the future, there is Hazelcast in Payara which supports storing timers as long as at least one cluster member is online...
Comments are very much appreciated. :-)
The text was updated successfully, but these errors were encountered: