Operating System: All
Platform: All

Attachment limited-length-mailbox-queue-not-handled.png has been added with description: New metric of "not-handled" for actors with mailbox type of "bounded-mailbox"

> After we limited the size of the mailbox queue

This sounds like you already have a fix ready. If you are willing to get this fix merged:
0. Click "take" in "Assigned To" field of this Bug and set "Status" to "in progress" to let other people know this Bug has a person dedicated to fixing it.
1. Upload your patch to Gerrit and add Controller committers as reviewers.
2. Post a comment here with the Gerrit link and set "Status" to "waiting for review".
3. Work with reviewers to get you patch merged, then set status to "fixed".

There are more verbose guides in various subpages of https://wiki.opendaylight.org/view/BestPractices

The problem with using a bounded mailbox is that change notifications will get dropped which likely isn't good either. You really should look into why the data tree change listener isn't keeping up and try to alleviate that (e.g. is it doing blocking operation(s) that could be done async?). Also maybe the front-end client(s) are producing transactions too fast.

> dropped messages due to the limited-size queue

Oh, I missed that. No application expects data tree changes to be dropped silently.

Would it be possible to block the committing thread (instead of dropping message) until queue is writable? Or would that lead to deadlocks (or some other queue exploding)?

(In reply to Vratko Polák from comment #3)
> > dropped messages due to the limited-size queue
>
> Oh, I missed that. No application expects data tree changes to be dropped
> silently.
>
> Would it be possible to block the committing thread (instead of dropping
> message) until queue is writable? Or would that lead to deadlocks (or some
> other queue exploding)?

Back pressure is the ideal way to do it but in this case the committing thread would block the shard from processing subsequent messages which isn't ideal. One slow listener could block raft heartbeats and subsequent transactions.

If a listener may do expensive processing or block I think it should offload that from the listener notification thread. If the queue or thread pool gets inundated then the listener logic can decide how best to handle that.

Just as you said, It's unacceptable to drop data tree change messages silenty when the message queue is fulfilled. So I would add a metric to record the counter of dropped messages by overriding the class MeteredBoundedMailbox.MeteredMessageQueue. The application can monitor this metric to see whether their listners works slowly or loses some critical data tree change messages.

And by means of this metric, we have found our bgp listeners consume data tree changes slowly than the production from the processing of bgp packets. We're now tring to offload the changes to a new queue and consolidate the changes.

The disadvantage of limiting the queue size I've met is that it will block the shard actor for a duration of timeout when the shard tries to tell a data tree change message to a fulfilled message queue. That'll decrease throughput of odl's distributed datastore. However this is a way of back-pressure to the producers.

This is a more complicated problem. The first it the transport part and is the inverse of BUG-5280, i.e. solution can use the same mechanism.

As for the backpressure problem, we cannot allow the system to grind to a halt simply because a bug in some obscure application. To that effect we have introduced DOMDataTreeListener, which has two things going for it:

• proper error callback, i.e. "you cannot keep up, I killed you, restart if you wish"
• ability to allow notifications to be state-compressed, hence doing a similar thing PingPongDataBroker does

These three combined can be used to:

• not overflow actor inbox (by exerting backpressure)
• keep the number of notifications low (state-compress them when they start piling up)
• if all else fails kill the listener

A further note: Data

ChangeListeners API contract does not allow a message to be dropped, as that amounts to data corruption.

You're right. Data corruption in application is unavoidable if the application consumes messages more slowly than the message producing from the distributed datastore.

After we limited the listener queue length, the bgp listeners cannot work correctly for they lost many data.

However, this will force such applications as bgp listeners to find the way to catch up with the producer. This's a good thing for it will improve the quality and throughput of whole system.

Regarding the complexity of applications, I don't think the distributed datastore can achieve the DataTreeChangeListener contract of no message lost or no data corruption.

The reasonable thing the distributed datastore can do on this issue is to keep more messages as possible as the JVM heap can allow rather than a fixed number of messages.

https://git.opendaylight.org/gerrit/#/c/49518/

Pushing the problem out to applications is unacceptable, simply because we do not give them the tools to solve the problem and it will lead to massive amounts of code duplication in the application layer, with the various workarounds having different kinds of bugs.

To illustrate on the specific example of BGP listeners, how do you propose they solve the problem, when they are completely unaware of the fact that the implementation has chosen to break the DTCN stream and hence the sum of observed state no longer matches what is in the data store?

I do not agree with your assessment of not being unable to fulfill a reasonable interface contract (where DOMDataTreeListener is reasonable). I believe we have all the tools we need to get the job done.

Is your assessment based on the CDS architecture or its implementation deficiencies?

Hi, Robert, What's CDS architecture? My assessment of bgp listeners comes from the performance testing of our bgp application customized from ODL's bgpcep project.

Currently, I don't have time and ability to implement the much better solution as you said in "Comment 6".

However, I look forward to seeing you or Tom can implement that solution to avoid using up JVM heap.

Before we can use that solution, I have to limit the queue size because using up JVM heap is a much more dangerous thing. We've suffered it once, and we won't suffer it anymore.