You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by "Noel J. Bergman" <no...@devtech.com> on 2006/09/02 17:22:43 UTC
Decision required for JAMES-603
The following is a summary of the problem.
1) It occurs ONLY when using JDBCSpoolRepository for RemoteDelivery
2) If there are more items in the spool than fit in the cache, it is
possible to delay delivery for messages that ought to be delivered.
3) If iterating through the cache takes more than one second, it is
possible to spinloop.
There are a variety of approaches. One is to fix it. So far neither
Stefano nor I (not that I've had much time to look, but he spent all day on
it), have come up with a trivial fix. The types of fixes for this code
would push back release for weeks. At that point I might as well implement
the right long-term change, planned for the next release, rather than a
one-off bandaid to resolve v2.3.
Alternatively, we could add a configuration parameter for the hardcoded
timeout value (there is already one for the cache size), document the
potential problem, and release JAMES v2.3.
I do not want to just remove the cache, which is one of Stefano's
suggestions. The cache prevents JAMES from crashing when the message
arrival rate is higher than it can process. Throwing OOMs and possibly
discarding messages in the process is not acceptable.
Recognize that part of the problem is the conflating of the RemoteDelivery
spool and the main pipeline spool, which have different requirements, since
the former applies scheduling on top of the spool. Again, that's on the
roadmap to change, but wasn't planned for v2.3.
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: Proposed fix for JAMES-603
Posted by Bernd Fondermann <be...@googlemail.com>.
> Shall we plan to release RC3 by end of week?
+1
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: Proposed fix for JAMES-603
Posted by Norman Maurer <nm...@byteaction.de>.
Am Mittwoch, den 06.09.2006, 09:03 +0200 schrieb Bernd Fondermann:
> > Shall we plan to release RC3 by end of week?
>
> +1
+1
Norman
Re: Proposed fix for JAMES-603
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
>> I think this patch should be committed to 2.3 branch
>
> Done to trunk and copied to 2.3. We don't want trunk to get left behind.
>>>From past experience, we know that it is important that changes to the
> branch be made in trunk so that things don't fork until we really intend to
> fork.
+1
>> we'll need a few weeks to update our production servers and have at
>> least a bunch of servers using the code we're going to release as
>> 2.3.0 before starting the vote.
>
> Shall we plan to release RC3 by end of week?
>
> --- Noel
+1
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: JAMES 2.3 RC3
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> Changes since RC2 are attached.
>
> I've done an upload to ~noel/james, but before I tag and we vote, is there
> anything else that anyone feels should go into the release?
>
> Last call at the saloon. :-)
>
> --- Noel
I updated JIRA to move issues from 2.3.0 to 2.3.0rc3.
Nothing more should end up in 2.3.0rc3, and let's hope this will be the
final candidate.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
JAMES 2.3 RC3
Posted by "Noel J. Bergman" <no...@devtech.com>.
Changes since RC2 are attached.
I've done an upload to ~noel/james, but before I tag and we vote, is there
anything else that anyone feels should go into the release?
Last call at the saloon. :-)
--- Noel
Re: Proposed fix for JAMES-603
Posted by Vincenzo Gianferrari Pini <vi...@praxis.it>.
Noel J. Bergman wrote:
>Shall we plan to release RC3 by end of week?
>
>
+1
Vincenzo
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: Proposed fix for JAMES-603
Posted by "Noel J. Bergman" <no...@devtech.com>.
Stefano Bagnara wrote:
> I reviewed the code and it seems ok.
> It is less than perfect but it is the less of the evil for fix this
> issue for 2.3.0.
:-)
> My only concern is about the removal of setMaxRows
So am I, but I didn't see an alternative that wouldn't restore the problem.
> I would like to know what happen on a mysql 3 with connector 3.0
> and big spools.
I have MySQL 3 and Connector/J 3.
> I think this patch should be committed to 2.3 branch
Done to trunk and copied to 2.3. We don't want trunk to get left behind.
>From past experience, we know that it is important that changes to the
branch be made in trunk so that things don't fork until we really intend to
fork.
> we'll need a few weeks to update our production servers and have at
> least a bunch of servers using the code we're going to release as
> 2.3.0 before starting the vote.
Shall we plan to release RC3 by end of week?
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: Proposed fix for JAMES-603
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> Attached is a first pass at an attempt to fix. I have reviewed it, and
> provided it directly to Stefano to review and try.
>
> I have NOT tried it on my system, yet, and won't put it on my production
> server for a while because I am trying to keep the current process going to
> verify the lack of memory leaks. But I want to get this out for review
> ASAP, before I have time to do some bench testing.
>
> --- Noel
I reviewed the code and it seems ok.
It is less than perfect but it is the less of the evil for fix this
issue for 2.3.0.
My only concern is about the removal of setMaxRows: unfortunately the
only big system where I plan to put this patch soon is on mysql5 and
connector/j 5 and I believe they correctly handle this scenario but I
would like to know what happen on a mysql 3 with connector 3.0 and big
spools.
I think this patch should be committed to 2.3 branch and that we'll need
a few weeks to update our production servers and have at least a bunch
of servers using the code we're going to release as 2.3.0 before
starting the vote.
I plan to be able to put this one on 3 servers: 2 using mysql (4 and 5)
and 1 using files only. Only the mysql 5 server have big traffic.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: Proposed fix for JAMES-603
Posted by "Noel J. Bergman" <no...@devtech.com>.
> Attached is a first pass at an attempt to fix.
#W@^%#W#W list filters! :-(
Ok, attached to JAMES-603 issue, and included for reference below (although
the mailing list will corrupt the formatting).
--- Noel
Index: src/java/org/apache/james/mailrepository/JDBCSpoolRepository.java
===================================================================
--- src/java/org/apache/james/mailrepository/JDBCSpoolRepository.java
(revision 439942)
+++ src/java/org/apache/james/mailrepository/JDBCSpoolRepository.java
(working copy)
@@ -189,12 +189,12 @@
//Loop through until we are either out of pending messages or
have a message
// that we can lock
PendingMessage next = null;
- while ((next = getNextPendingMessage()) != null &&
!Thread.currentThread().isInterrupted()) {
+ while ((next = getNextPendingMessage(filter)) != null &&
!Thread.currentThread().isInterrupted()) {
//Check whether this is time to expire
- boolean shouldProcess = filter.accept (next.key,
next.state, next.lastUpdated, next.errorMessage);
+ // boolean shouldProcess = filter.accept (next.key,
next.state, next.lastUpdated, next.errorMessage);
- if (shouldProcess && lock(next.key)) {
+ if (/*shouldProcess && */ lock(next.key)) {
try {
Mail mail = retrieve(next.key);
// Retrieve can return null if the mail is no
longer on the spool
@@ -241,11 +241,12 @@
* checks the last time pending messages was loaded and load if
* it's been more than 1 second (should be configurable).
*/
- private PendingMessage getNextPendingMessage() {
+ private PendingMessage
getNextPendingMessage(SpoolRepository.AcceptFilter filter) {
synchronized (pendingMessages) {
if (pendingMessages.size() == 0 && pendingMessagesLoadTime <
System.currentTimeMillis()) {
- pendingMessagesLoadTime = LOAD_TIME_MININUM +
System.currentTimeMillis();
- loadPendingMessages();
+ // pendingMessagesLoadTime = LOAD_TIME_MININUM +
System.currentTimeMillis();
+ loadPendingMessages(filter);
+ pendingMessagesLoadTime = Math.max(filter.getWaitTime(),
LOAD_TIME_MININUM) + System.currentTimeMillis();
}
if (pendingMessages.size() == 0) {
@@ -259,7 +260,7 @@
/**
* Retrieves the pending messages that are in the database
*/
- private void loadPendingMessages() {
+ private void loadPendingMessages(SpoolRepository.AcceptFilter filter) {
//Loads a vector with PendingMessage objects
synchronized (pendingMessages) {
pendingMessages.clear();
@@ -272,7 +273,11 @@
listMessages =
conn.prepareStatement(sqlQueries.getSqlString("listMess
agesSQL", true));
listMessages.setString(1, repositoryName);
- listMessages.setMaxRows(maxPendingMessages);
+ // Too simplistic. When filtering, we may need to see
+ // more than just maxPendingMessages to load the
+ // cache, so just hope that the driver and server use
+ // cursors properly.
+ // --> listMessages.setMaxRows(maxPendingMessages);
rsListMessages = listMessages.executeQuery();
// Continue to have it loop through the list of messages
until we hit
// a possible message, or we retrieve maxPendingMessages
messages.
@@ -283,7 +288,9 @@
String state = rsListMessages.getString(2);
long lastUpdated =
rsListMessages.getTimestamp(3).getTime();
String errorMessage = rsListMessages.getString(4);
- pendingMessages.add(new PendingMessage(key, state,
lastUpdated, errorMessage));
+ if (filter.accept(key, state, lastUpdated,
errorMessage)) {
+ pendingMessages.add(new PendingMessage(key, state,
lastUpdated, errorMessage));
+ }
}
} catch (SQLException sqle) {
//Log it and avoid reloading for a bit
Proposed fix for JAMES-603
Posted by "Noel J. Bergman" <no...@devtech.com>.
Attached is a first pass at an attempt to fix. I have reviewed it, and
provided it directly to Stefano to review and try.
I have NOT tried it on my system, yet, and won't put it on my production
server for a while because I am trying to keep the current process going to
verify the lack of memory leaks. But I want to get this out for review
ASAP, before I have time to do some bench testing.
--- Noel
Re: Decision required for JAMES-603
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> Stefano Bagnara wrote:
>
>> Noel J. Bergman wrote:
>>> OK, I believe that I have a "simple" solution, which I will try and
>>> hopefully commit tomorrow.
>>>
>>> I'm going to push the filter further into the code where we can
>>> effect the cache directly.
>
>> This sound similar to my first approach:
>
>> I changed getNextPendingMessage to getNextAvailableMessage(Filter) and
>> moved the accept inside the method so that I have the accept in the same
>> code where we manage the pendingMessages cache.
>
> I'm pushing the filter all the way down to where we actually populate the
> cache, so that loadPendingMessages(Filter) will populate the cache with up
> to the first <maxcache> *accepted* messages from the ResultSet, rather than
> consume the cache with currently unacceptable messages.
>
> --- Noel
Yes, my first try. But the problem is the waitTime calculation.
To calculate the waitTime the filter have to be called ALSO for each
unacceptable message in the spool and if you remove them from the cache
it won't know how much to wait... How did you solve this?
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: Decision required for JAMES-603
Posted by "Noel J. Bergman" <no...@devtech.com>.
Stefano Bagnara wrote:
> Noel J. Bergman wrote:
> > OK, I believe that I have a "simple" solution, which I will try and
> > hopefully commit tomorrow.
> >
> > I'm going to push the filter further into the code where we can
> > effect the cache directly.
> This sound similar to my first approach:
> I changed getNextPendingMessage to getNextAvailableMessage(Filter) and
> moved the accept inside the method so that I have the accept in the same
> code where we manage the pendingMessages cache.
I'm pushing the filter all the way down to where we actually populate the
cache, so that loadPendingMessages(Filter) will populate the cache with up
to the first <maxcache> *accepted* messages from the ResultSet, rather than
consume the cache with currently unacceptable messages.
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: Decision required for JAMES-603
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> OK, I believe that I have a "simple" solution, which I will try and
> hopefully commit tomorrow.
>
> Rather than keep you in suspense, I'm going to push the filter further into
> the code where we can effect the cache directly.
>
> --- Noel
This sound similar to my first approach:
I changed getNextPendingMessage to getNextAvailableMessage(Filter) and
moved the accept inside the method so that I have the accept in the same
code where we manage the pendingMessages cache. Then I thought at it and
I only found a solution that need a change to the accept method to
return the next timestamp instead of boolean. This is easy because we
already calculate the next time in all of our acceptfilters, but need an
interface change.
Unfortunately yesterday I had no time/soul for code.
I'm looking forward for your solution!
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: Decision required for JAMES-603
Posted by "Noel J. Bergman" <no...@devtech.com>.
OK, I believe that I have a "simple" solution, which I will try and
hopefully commit tomorrow.
Rather than keep you in suspense, I'm going to push the filter further into
the code where we can effect the cache directly.
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
RE: Decision required for JAMES-603
Posted by "Noel J. Bergman" <no...@devtech.com>.
OK, I am done with the things that were consuming my time for the past
weeks, so I will take a closer look at the current spooling situation, and
see if I can resolve the problem.
--- Noel
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org
Re: Decision required for JAMES-603
Posted by Stefano Bagnara <ap...@bago.org>.
Noel J. Bergman wrote:
> The following is a summary of the problem.
>
> 1) It occurs ONLY when using JDBCSpoolRepository for RemoteDelivery
> 2) If there are more items in the spool than fit in the cache, it is
> possible to delay delivery for messages that ought to be delivered.
> 3) If iterating through the cache takes more than one second, it is
> possible to spinloop.
I'm investigating further on the problem. I had this again today, even
if I raised the maxcache to 10000 and I had less than 10000 messages. So
something weird is happening and I have to check it better.
Furthermore I'm under the impression that I have a similar issue also on
the main spool manager... Maybe there are multiple problems so I have to
fix some of them to check for the others.
> There are a variety of approaches. One is to fix it. So far neither
> Stefano nor I (not that I've had much time to look, but he spent all day on
> it), have come up with a trivial fix. The types of fixes for this code
> would push back release for weeks. At that point I might as well implement
> the right long-term change, planned for the next release, rather than a
> one-off bandaid to resolve v2.3.
The long term change needs a db change and we decided to keep db
structure unchanged until 3.0 so imho we need a fix for 2.3 and 2.4 that
doesn't include changing the db to replace the last_updated with
next_processing_time or something similar.
> Alternatively, we could add a configuration parameter for the hardcoded
> timeout value (there is already one for the cache size), document the
> potential problem, and release JAMES v2.3.
Imho the problem is not the timeout: the timeout is there to avoid that
all the threads run the same query on the repository when there are no
messages. Without timeout you would need 50 queries to decide that you
have nothing to do, with the timeout this is fixed. Increasing the
timeout is an hack and would work only because we already have an hack
that our threads wake up every 60 seconds (we don't do this for file
repositories that works better regarding to this issue).
> I do not want to just remove the cache, which is one of Stefano's
> suggestions. The cache prevents JAMES from crashing when the message
> arrival rate is higher than it can process. Throwing OOMs and possibly
> discarding messages in the process is not acceptable.
I think that now we have a behaviour that is buggy and difficult to
understand and to solve. I want to have it fixed on my system before
deciding what to do with 2.3.0.
And my preferred solution, now, is not the removal of the cache but a
complete rewrite of the chaching algorythm and the accept mechanism
without changing the db.
> Recognize that part of the problem is the conflating of the RemoteDelivery
> spool and the main pipeline spool, which have different requirements, since
> the former applies scheduling on top of the spool. Again, that's on the
> roadmap to change, but wasn't planned for v2.3.
>
> --- Noel
Well, we have a bug and we may need to change the original plan.
I still think there is something more about this issue to be discovered
so I will talk about possible solutions later, when I'll have
investigated a little more on this hard issue.
Stefano
---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org