Provide an interface for generating sequences#678
Provide an interface for generating sequences#678VladislavBakshanskij wants to merge 7 commits intogruelbox:masterfrom
Conversation
VladislavBakshanskij
changed the title
|
@badgerwithagun can u review? |
| * @param sequenceGenerator The sequence generator used for ordered tasks. Required | ||
| */ | ||
| @SuppressWarnings("JavaDoc") | ||
| private final SequenceGenerator sequenceGenerator; |
There was a problem hiding this comment.
On its own, this is a compatibility break; it requires all existing users to specify DefaultSequenceGenerator when creating a DefaultPersistor. I suggest changing this class to have a constructor annotated with @Builder; then you can accept null for sequenceGenerator and replace it with a DefaultSequenceGenerator.
| .persistor( | ||
| DefaultPersistor.builder() | ||
| .dialect(connectionDetails().dialect()) | ||
| .sequenceGenerator(DefaultSequenceGenerator.builder() |
There was a problem hiding this comment.
This shouldn't be necessary (see comments on DefaultPersistor)
| // used correctly. | ||
| .dialect(Dialect.POSTGRESQL_9) | ||
| .dialect(dialect) | ||
| .sequenceGenerator(DefaultSequenceGenerator.builder().dialect(dialect).build()) |
There was a problem hiding this comment.
This shouldn't be necessary (see comments on DefaultPersistor)
| return DefaultPersistor.builder().dialect(dialect).build(); | ||
| return DefaultPersistor.builder() | ||
| .dialect(dialect) | ||
| .sequenceGenerator(DefaultSequenceGenerator.builder().dialect(dialect).build()) |
There was a problem hiding this comment.
This shouldn't be necessary (see comments on DefaultPersistor)
badgerwithagun
left a comment
badgerwithagun
left a comment
There was a problem hiding this comment.
One tweak needed to avoid any compatibility issues, but otherwise this looks fine.
I must caution you though: if you're planning on generating sequence numbers from outside the main DB transaction, you're going to get some unexpected timing differences.
There are several combinations to consider, but here's one:
- Thread A asks for a sequence number and gets 1
- Thread B asks for a sequence number and gets 2
- Thread B writes its outbox record (sequence 2) and commits
flush()is called- (2) is picked up and processed
- Thread A writes its outbox record (sequence 1) and commits
flush()is called- (1) is picked up and processed
The order of processing was actually 2, 1 instead of 1, 2.
So that's a bit potentially confusing, but another way of looking at this problem is that:
DefaultSequenceGeneratorensures that the order of commit of the source transactions align with the order of the outgoing messages.- A Redis implementation could only ensure that the order of the outgoing messages aligns with the point in time that the sequence number was requested.
VladislavBakshanskij
force-pushed
the
master
branch
from
a2d4ea8 to
690545b
Compare
|
@badgerwithagun I came back late of course with the fix of the comments, but I still corrected them. I ask you to do a second review |
VladislavBakshanskij
force-pushed
the
master
branch
from
ee67b1c to
86378f8
Compare
VladislavBakshanskij
force-pushed
the
master
branch
from
e5e8905 to
cacd8da
Compare
|
@badgerwithagun can u review this PR? |
2 participants
fixes: #677