Implement and use a Session data storage #721
Conversation
andsel
force-pushed
the
feature/persists_session_data_storage
branch
from
165cafb to
1162c29
Compare
…d included JetBrains null/nullable annotations library
andsel
force-pushed
the
feature/persists_session_data_storage
branch
from
1162c29 to
69ae603
Compare
|
Hi @hylkevds I would like to ask if you could be the reviewer for this PR? |
| @@ -46,6 +46,7 @@ | |||
| class Session { | |||
|
|
|||
| private static final Logger LOG = LoggerFactory.getLogger(Session.class); | |||
| static final int INFINITE_EXPIRY = 0xFFFFFFFF; | |||
There was a problem hiding this comment.
It's probably better to use Integer.MAX_VALUE instead of a magic number.
There was a problem hiding this comment.
It's not a magic number, it's simply an unsigned 32 bit full of 1, which 4,294,967,295, however 32 bit int ranges between 2,147,483,647 and -2,147,483,648. We should save it in a long to do not loose the sign.
If we use Integer.MAX_VALUE we would loose all the values between 2,147,483,647 and 4,294,967,295 so if we go for it, we loose last bit. Given that unitis ~136 years andInteger.MAX_VALUE` is ~68 years, I think we could use the signed integer max value.
There was a problem hiding this comment.
That's pretty much exactly the definition of a "magic number" and why they are a bad idea :)
If you change it to a long, using Long.MAX_VALUE would be better too.
But regardless, there are clearer ways to specify the default max session duration. Something like:
Duration.ofDays(365*100).toMillis()
That both makes clear that the result is in milliseconds, and 100 years. Unfortunately the Java time classes are not very good, so saying Duration.of(100, YEARS) doesn't work.
| @@ -236,6 +243,7 @@ private Session createNewSession(MqttConnectMessage msg, String clientId) { | |||
| } | |||
|
|
|||
| newSession.markConnecting(); | |||
| sessionsRepository.saveSession(new ISessionsRepository.SessionData(clientId, Instant.now(), MqttVersion.MQTT_3_1_1, INFINITE_EXPIRY)); | |||
There was a problem hiding this comment.
Never expiring sessions are a big problem for public-facing brokers. Users connecting with random UUIDs and cleanSession=false will fill up all space until the broker crashes. In FROST I'm using a modified Moquette for this reason.
A configurable default that is not infinite would be really appreciated.
There was a problem hiding this comment.
That's part of MQTT5 migration, this PR is just to be feature parity with MQTT 3.1 but introducing the session's store.
We could address this in a follow up PR, so that setting could be read from the moquette.conf, but in general is a feature that's part of MQTT5 specification.
| private final String clientId; | ||
| private final Instant created; | ||
| final MqttVersion version; | ||
| final long expiryInterval; |
There was a problem hiding this comment.
The expiry code is not there yet, but which time instant will the expiryInterval be evaluated against?
Calculating it against the "created" time of the session makes little sense. I would expect it to be evaluated against the disconnect time, to get the expire time. But that would require the disconnect time in the SessionData, not the created time.
Alternatively, instead of an expiryInterval, we could store the expireInstant at which the session will expire, and set that when the connection is lost.
Introduce the concept of SessionsRepository to store session's data that are not subscriptions or queues; those are already persisted with their repository instances.
This is a step to move to MQTT5 which benefit also the MQTT3 implementation.
What does it do?
ISessionRepositoryinterface and its H2 implementationSessionDataSessionDatafor H2Task list
SessionDatareference insideSessionfor the data part, to avoid moving data between object instances