Rewrite LogStorageAppenderTest

[ChrisKujawa]

Description

Currently, the LogStorageAppenderTest is tightly coupled with implementation details. During moving the AtomixLogStorage #9235 we had to move the test as well and open the Writer implementation, which is sub-optimal.

We have to reevaluate again what we actually want to test here. The inconsistency check? Which is part of the ZeebeValidator and LeaderAppender? Then I guess we can test this differently, no need to use the LogStorageAppender here.

Potentially we can split up the test class and move parts back to the Logstreams tests and leave some in the broker to test the inconsistency check.

Related comment #9235 (comment)

[npepinpe]

So I'll note that some observation.

1. The LogStorageAppenderTest should 100% be in the logstreams module. The only test which prevents this is the invalid entry one, which can be tested as you mentioned directly with the entry validator.
2. The ZeebeEntryValidator is a bit of a headache. It needs to know about ApplicationEntry (so Raft specific stuff), but also wants to read the actual logstream entry (as seen by its usage of LogEntryDescriptor). This sort of couples it to the implementation of the logstream. This is currently necessary because it iterates through the entry buffer and checks for gaps in the positions. We could replace this with just comparing the ApplicationEntry#lowestPosition/highestPosition of the both entries, but that check is less strict than actually going through the buffer and checking the actual positions for gaps. For example, you could have an ApplicationEntry with lowest = 10 and highest = 20, but then maybe one of the log entries in it has the wrong position.
3. The LogStorageAppender is over complicated. It could be simplified by first splitting the non-actor logic from the actor specific logic, e.g. LogStorageAppenderActor deals with lifecycle, consuming channel, timers, etc., and LogStorageAppender is just appending things without any actor knowledge.
4. The LogStorageAppender is tightly coupled to the Dispatcher, even though it actually only uses 1 method (peekBlock) and one class (BlockPeek), and even in that class it only uses one method getRawBuffer). This makes writing tests for it heavier, more complex, and flakier - you need to deal with a complete dispatcher. Instead, you could have Subscription implement a simple interface, e.g. ConsumableStream with the peekBlock method, and have BlockPeek implement a simple interface PeekBuffer with a single method getReadBuffer() which would return a read-only copy of the raw buffer. This would greatly simplify the tests and increase maintainability.
5. I notice that the engine uses LogStreamBatchReaderImpl directly instead of getting it from the LogStream, which breaks abstraction.

Anyway, my proposal is:

• Remove the shouldDetectInvalidEntry test from the LogStorageAppenderTest, move LogStorageAppenderTest back to logstreams.
• For now, rewrite the ZeebeEntryValidator to just check the two ApplicationEntry's highest and lowest positions, and not know how to read the log entries.
• Add a ZeebeEntryValidatorTest which tests that invalid entries are properly detected.

This would be the scope of this issue. I would propose a follow up issue to deal with the ZeebeEntryValidator. The crux of the issue here is that knowing how to read log entries is implementation detail of the log stream implementation, but we register/create the ZeebeEntryValidator when building the Raft instance. Since the LogStream implementation relies on the Raft itself as its log storage, there's a chicken and egg problem. I would propose instead to have the entry validator be register-able by the LogStream once it is installed. Conceptually I believe it makes sense - the log stream is installed on the Raft (with the implied assumption that it is the ONLY thing writing ApplicationEntry of course), and only it knows how to validate its own entries. This means there needs to be some new interface in the LogStorage to register an entry validator.

I would also propose to move the logic of validating entries in the storage itself. Right now this is done in the ZeebeLogAppender/LeaderRole, but it's unclear to me why this is necessary, as we don't use these anywhere else. My only argument here is this makes everything simpler (e.g. you register it in the storage, and then use right there in the append method - no need to propagate it), but I might be missing something.

[npepinpe]

To be honest we could also do the second issue first (e.g. moving the validator to logstreams), which then avoids having to rewrite the test a second time.