Project

General

Profile

AMP expired processor

Robert Larsen
Added over 4 years ago

I'm seeing around 10.000 database queries per second all asking for expired messages so I tracked this down:

expiredProcessor = new Thread("expired-processor") {
    @Override
    public void run() {
        while (true) {
            Element elem = repo.getMessageExpired(0, true);

            if (elem != null) {
                elem.addAttribute(OFFLINE, "1");
                elem.addAttribute(EXPIRED, "1");
                try {
                    resultsHandler.addOutPacket(Packet.packetInstance(elem));
                } catch (TigaseStringprepException ex) {
                    log.info("Stringprep error for offline message loaded from DB: " + elem);
                }
            }
        }
    }
};

No pause between iterations, not even if there were no more expired messages.

Is this really necessary?


Replies (4)

Added by Robert Larsen over 4 years ago

Also, I see that the implementation of getMessageExpired(long time, boolean delete) in tigase.server.amp.MsgRepository does not take the 'time' parameter into account. Is that intentional?

I would assume that 'repo.getMessageExpired(0, true);' should really return no messages because no message should expire at or before time zero. Or is the time relative to System.currentTimeMillis() so zero means 'now'?

Avatar?id=6023&size=32x32

Added by Artur Hefczyc TigaseTeam over 4 years ago

From your second post I guess you found an answer to the first question: repo.getMessageExpired(...) should wait until there is some data to return. And this depends on the MsgRepositoryIfc implementation. I guess, if you have your own implementation of the repository and it returns without waiting then you have this loop consuming resources.

Answering second question. The time argument really means: return all messages which will be expired within given number of milliseconds. So if you put 0, it means, return all messages which are already expired. If you put 10, it means return all messages which will be expired in 10 milliseconds, if you put 1,000 - all messages which will expired in 1 second. Currently this time argument is not really used as we only read messages which are already expired. I added the argument during implementation thinking that it might be a way to optimize DB access in some cases. However, DB performance for AMP was never such an issue worth adding extra optimization.

I hope this answers your both questions. If not, please let me know.

Added by Robert Larsen over 4 years ago

Excellent, that answers my question. I don't think it was clear from the getMessageExpired() JavaDoc that it should block until an expired message was found.

Avatar?id=6023&size=32x32

Added by Artur Hefczyc TigaseTeam over 4 years ago

Indeed, I have updated JavaDoc to make it clear.

    (1-4/4)