Yicong-Huang commented on code in PR #8242:
URL: https://github.com/apache/texera/pull/8242#discussion_r4365121342
##########
amber/src/main/python/core/architecture/sendsemantics/hash_based_shuffle_partitioner.py:
##########
@@ -28,6 +28,8 @@
class HashBasedShufflePartitioner(IndexedShufflePartitioner):
+ _clear_batch_on_flush = True
Review Comment:
**Polish:**
`round_robin_partitioner.py:30` still says "Unlike the hash/range shuffles,
round-robin drains a slot's batch on flush." This change makes that comment
false. Removing the flag (see the other comment here) would remove it too.
##########
amber/src/test/python/core/architecture/sendsemantics/test_partitioners.py:
##########
@@ -289,15 +289,14 @@ def
test_flush_emits_pending_batch_and_ecm_for_target_only(self):
ecm = EmbeddedControlMessage()
a_out = _snapshot(p.flush(p.receivers[0][0], ecm))
assert a_out == [[_hashable_tuple(k=1)], ecm]
+ assert p.receivers[0][1] == []
- def test_flush_does_not_clear_the_flushed_slot(self):
- # Hash/range shuffles leave a flushed slot's batch intact
- # (_clear_batch_on_flush=False), unlike round-robin.
+ def test_flush_clears_the_flushed_slot(self):
p = self._partitioner(batch_size=10)
p.receivers[0] = (p.receivers[0][0], [_hashable_tuple(k=1)])
ecm = EmbeddedControlMessage()
list(p.flush(p.receivers[0][0], ecm))
- assert p.receivers[0][1] == [_hashable_tuple(k=1)]
+ assert p.receivers[0][1] == []
Review Comment:
**Advisory:**
This test now makes the same `p.receivers[0][1] == []` assertion as line
292, and both check internal state only. Replacing it with the #8241 repro
(flush twice, the second flush yields only the ECM) would pin the behavior the
issue reports. Asserting the non-target slot is untouched, as the range test
does at line 385, would also cover the property the description claims.
##########
amber/src/main/python/core/architecture/sendsemantics/hash_based_shuffle_partitioner.py:
##########
@@ -28,6 +28,8 @@
class HashBasedShufflePartitioner(IndexedShufflePartitioner):
+ _clear_batch_on_flush = True
Review Comment:
**Advisory:**
With this PR all three `IndexedShufflePartitioner` subclasses set
`_clear_batch_on_flush = True`, so the `False` default at
`indexed_shuffle_partitioner.py:42` has no user left. That default is exactly
the duplicating behavior from #8241, so a future subclass that omits the flag
brings the bug back silently. Consider deleting the flag and always draining in
the base class.
--
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]
