davidzollo commented on PR #10849:
URL: https://github.com/apache/seatunnel/pull/10849#issuecomment-4756671425
Thanks again for the contribution. I took one more careful pass over the
engine-side threading model and the current source implementation, and I think
the follow-up here can stay focused and simple.
Since this PR is already merged, I do think it would be worth sending a
small follow-up PR to harden the source-side state synchronization.
To keep the implementation easy to reason about, I would recommend using a
plain `private final Object stateLock = new Object();` in the affected
source-side classes. I do not think this needs `ReentrantLock`, and I would
avoid line-by-line synchronization.
The places I would explicitly protect are:
1. `BigtableSourceReader`
- Add a small `stateLock`
- Use it in:
- `pollNext(...)`: only around the reader state transition block that
polls `pendingSplits`, assigns `currentSplit`, and clears `currentSplit` after
the split read completes
- `snapshotState(...)`
- `addSplits(...)`
- I would keep the actual `readRows(...)` network scan outside this lock
- I would also keep the existing `output.getCheckpointLock()` only around
`output.collect(...)`
- I do not think `handleNoMoreSplits()` needs to join this lock for now;
the current `volatile` flag is enough there
2. `BigtableSourceSplitEnumerator`
- Add a small `stateLock`
- Use it in:
- `addSplitsBack(...)`
- `registerReader(...)`
- `snapshotState(...)`
- The shared state that should be protected by that lock is:
- `assignedSplits`
- `pendingSplits`
- `initialized`
- `initializePendingSplits()` and `assignSplit()` should then run only
while this lock is already held
- I do not think this follow-up needs to expand to `open()`, `run()`,
`close()`, `handleSplitRequest()`, or `currentUnassignedSplitSize()`
3. `BigtableSourceState`
- Please make a defensive copy of `assignedSplits` when building the
checkpoint state, instead of storing the live set reference directly
Summary:
- Lock type: plain `Object stateLock`
- Reader methods to protect:
- `pollNext(...)` state-transition block
- `snapshotState(...)`
- `addSplits(...)`
- Enumerator methods to protect:
- `addSplitsBack(...)`
- `registerReader(...)`
- `snapshotState(...)`
- State object fix:
- make `BigtableSourceState` hold a copied split set, not the live set
reference
- I do not think sink-side locking changes are needed in this follow-up
If you’re available, would you mind sending a small follow-up PR for this?
Since the current PR is already merged, a focused patch just for these
source-side synchronization points would be the cleanest next step.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
