moquette-io / moquette

Java MQTT lightweight broker
http://moquette-io.github.io/moquette/
Apache License 2.0
2.27k stars 814 forks source link

Create custom abstraction for session's message queues #693

Closed andsel closed 1 year ago

andsel commented 1 year ago

What it does?

Introduce a simplified abstraction for Session's message queues. Instead of using Java Collections' framework Queue abstraction which has many operations not used, for example iteration without consuming (peek) or size retrieval this commit introduce a much simpler interface based on 4 methods:

hylkevds commented 1 year ago

Yes, great idea! I was just looking at #694, and this is also the solution to that issue.

Sessions manage their queues, so the Session should remove its queue in the cleanup() method. However, to remove a queue from its Queuerepostory, we need a reference to the queue repository. Adding that reference to the Session is ugly. But since this PR adds a close() method to the queue, this close method can also delete the queue from its repository. Giving the queue a reference to the repository that holds the queue does make sense.

A JavaDoc issue: Are these queues supposed to be thread safe? I think they do not need to be in our current design, so it's probably a good idea to explicitly state that the queue is not thread safe?

Another suggestion for the JavaDoc on the close() method: A closed queue will not accept new items and will be removed from the repository.

andsel commented 1 year ago

I think that close() just close the queue, to clean up the content from it's better to be explicit and introduce a purge() method that could be called only a closed queue. I've a mixed feeling about this, maybe just a renamed from close to closeAndPurge is better.

hylkevds commented 1 year ago

What's the use-case for closing a queue but not purging it? Closing the queue already clears all content from it, so there's no data left to purge, only the reserved space in the queue store.

I could see a use if closing a queue stops new data from being added, but does not delete the content yet. Then one can first close the queue, wait a bit for the content to be handled, before purging the queue to deleting the remaining data.

andsel commented 1 year ago

What's the use-case for closing a queue but not purging it?

Actually we haven't but in general a closed persistent queue is queue where it's memory data structures are released but could be reopened.

Closing the queue already clears all content from it, so there's no data left to purge, only the reserved space in the queue store.

This is partially true in the actual in memory implementation, where the messages are released but the queue instance is not removed from the repository.

However I agree that close should free the space, but it's better to rename to closeAndPurge to make it more evdent.

hylkevds commented 1 year ago

closeAndPurge sounds good, since we don't re-open the queues. If we ever have the need to close and reopen queues we can add separate close, reOpen and purge methods.

andsel commented 1 year ago

@hylkevds with commit 3bce16db879c3471a8dcf046ac145c0e1b520ceb I've added close check status before every queue operation.