Conversation
…est-ecs-state-store
ca61688
added
the
needs-reviewer
| // been written to the config bucket yet when we first start up. | ||
| if (response.hasMessages()) { | ||
| if (!isProcessingTasks) { | ||
| LOGGER.info("State store committer process started at {}", Instant.now()); |
There was a problem hiding this comment.
It doesn't seem like we'll always get this log in a system test? When one test finishes, it'll move straight on to the next, and it seems like it could submit more messages to the queue before the isProcessingTasks flag gets unset.
It seems like it would be more reliable to process the logs after the fact rather than have logic in the committer for whether it should make the log in the first place? There's already a log just below here that happens every time we receive messages. We could process that rather than adding an extra one.
| private StateStoreProvider stateStoreProvider; | ||
| private RetryOnThrottling retryOnThrottling; | ||
| private Map<String, CompletableFuture<Instant>> tableFutures = new HashMap<>(); | ||
| private boolean isProcessingTasks = false; |
There was a problem hiding this comment.
This field name is a bit confusing. This class doesn't process tasks, it runs in a task. It processes state store commits.
| /** | ||
| * A log entry recording that a state store committer batch finished processing. | ||
| */ | ||
| public class StateStoreCommitterRunBatchFinished extends StateStoreCommitterRunFinished { |
There was a problem hiding this comment.
It doesn't look like there are any tests for the handling of this new log type, or for handling the logs that will be emitted overall by the ECS-based committer.
The existing unit tests are in:
- ReadStateStoreCommitterLogsTest
- StateStoreCommitterRunsFromEntriesTest
- StateStoreCommitterRequestsPerSecondTest
patchwork01
removed
the
needs-reviewer
2 participants
Make sure you have checked all steps below.
Issue
Feature". Note that before an issue is finished, you can still make a pull request by raising a separate issue
for your progress.
Tests
Documentation
separate issue for that below.