Wrong computation of notification logs

[mwillema]

I have discovered an insidious bug in NotificationLogFactory.calculateCurrentNotificationLogId.

Indeed, for this to work, we assume that stored event ids are generated by the database and that they is never any gap between them. However this is not a valid assumption as indicated in the MySQL documentation:

In all lock modes (0, 1, and 2), if a transaction that generated auto-increment values rolls back, those auto-increment values are “lost.” Once a value is generated for an auto-increment column, it cannot be rolled back, whether or not the “INSERT-like” statement is completed, and whether or not the containing transaction is rolled back. Such lost values are not reused. Thus, there may be gaps in the values stored in an AUTO_INCREMENT column of a table.

Consider this example: we have 20 stored events, the first identified by 1 and the last by 21. Because at some point we had to rollback, the generated id 3 is lost. So the stored events have the following ids: 1, 2, 4, 5,... up to 21. Also we have NOTIFICATIONS_PER_LOG = 20.

Now in the calculateCurrentNotificationLogId method the variables take the following values:

count = 20
remainder = count % NOTIFICATIONS_PER_LOG = 0

// since remainder == 0 && count > 0
remainder = NOTIFICATIONS_PER_LOG = 20

low = count - remainder + 1 =  20 - 20 + 1 = 1
high = low + NOTIFICATIONS_PER_LOG - 1 = 20

So the current notificationLogId is [1-20], which will return the stored events between ids 1 and 20. So this log will only contain 19 notifications and the last stored event will be ignored.

I have found two solutions to fix this problem:

1. Add the lastStoredEventId() method to EventStore and replace the calls to countStoredEvents() with lastStoredEventId().

This way we don't use the number of stored events to calculate the current notification log and we don't skip any stored event anymore. However we have to accept that not all archived notification logs will contain exactly NOTIFICATIONS_PER_LOG notifications.

Note that in some extreme cases (with many rollbacks), we may even have empty notification logs. But as long as we don't rely on the exact number of notifications per log when we consume them, I think this solution is fine. This is what I have implemented in my company.

2. Still use the countStoredEvents() method, but replace the allStoredEventsBetween(long aLowStoredEventId, long aHighStoredEventId) by something like allStoredEventsBetween(long aLowIndex, long aHighIndex) or allStoredEvents(long anOffset, long aLimit).

This way we don't rely on the stored event ids and we have no gap between the notification ids, i.e., the archived notification logs will always contain NOTIFICATIONS_PER_LOG notifications.

However for any given notification, its id will not always match the id of its encapsulated event. On one hand we can argue that this is fine, since it doesn't expose how stored events are identified internally. On the other hand, for debugging or other purposes, we may like the fact that a notification id reflects the stored event id.

So what do you think about that?