MLE-25782 Some refactoring of BatchWriter #1858
Conversation
|
Copyright Validation Results ✅ Valid Files
✅ All files have valid copyright headers! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the BatchWriter class by extracting it from WriteBatcherImpl into its own file and improves the immutability of BatchWriteSet by making several fields final. The changes streamline the code structure without altering functionality.
Key changes:
- Extracted
BatchWriterinner class to a separate top-level class - Made multiple fields in
BatchWriteSetfinal and removed their setters - Updated
BatchWriteSetconstructor to acceptbatchNumberparameter directly
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| WriteBatcherImpl.java | Removed BatchWriter inner class and updated references to use getter method; updated BatchWriteSet instantiation to pass batchNumber in constructor |
| BatchWriter.java | New file containing extracted BatchWriter class with package-private visibility and added getter for writeSet |
| BatchWriteSet.java | Made fields final, removed setters, updated constructor to accept batchNumber, removed unused onBeforeWrite functionality, and changed class to package-private |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public void run() { | ||
| try { | ||
| logger.trace("begin write batch {} to forest on host \"{}\"", writeSet.getBatchNumber(), writeSet.getClient().getHost()); |
There was a problem hiding this comment.
The onBeforeWrite callback that was present in the original implementation has been removed. If this callback was being used elsewhere in the codebase, its removal could break existing functionality. Verify that onBeforeWrite was truly unused before removing this feature.
| logger.trace("begin write batch {} to forest on host \"{}\"", writeSet.getBatchNumber(), writeSet.getClient().getHost()); | |
| logger.trace("begin write batch {} to forest on host \"{}\"", writeSet.getBatchNumber(), writeSet.getClient().getHost()); | |
| Runnable onBeforeWrite = writeSet.getOnBeforeWrite(); | |
| if (onBeforeWrite != null) { | |
| onBeforeWrite.run(); | |
| } |
| this.client = client; | ||
| this.transform = transform; | ||
| this.temporalCollection = temporalCollection; | ||
| this.batchNumber = batchNumber; |
There was a problem hiding this comment.
Inconsistent indentation: this line uses tabs while most other lines in the constructor use spaces. Standardize to match the project's indentation style.
| this.batchNumber = batchNumber; | |
| this.batchNumber = batchNumber; |
rjrudin
force-pushed
the
feature/batch-listener
branch
2 times, most recently
from
6678ff7 to
ed87416
Compare
Wanted to move this into a separate class so it's easier / cleaner to prototype an incremental check listener. Also made BatchWriteSet nicer by making a bunch of fields final. No change in functionality, just cleaning up code.
rjrudin
force-pushed
the
feature/batch-listener
branch
from
ed87416 to
5bf5f48
Compare
Wanted to move this into a separate class so it's easier / cleaner to prototype an incremental check listener. Also made BatchWriteSet nicer by making a bunch of fields final. No change in functionality, just cleaning up code.