[BitSail][Connector] Support Assert sink connector #282
Conversation
There was a problem hiding this comment.
thanks for your contribution @liuxiaocs7 , left some comments
| @@ -0,0 +1,9 @@ | |||
| { | |||
| "name": "bitsail-connector-unified-assert", | |||
There was a problem hiding this comment.
should we remove unified here?
There was a problem hiding this comment.
I noticed that connector V1 is uniformly named bitsail-connector-unified-xxx, like this, the same naming convention is used here.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class AssertRuleExecutor { |
There was a problem hiding this comment.
can we add UT for this class?
There was a problem hiding this comment.
sure, i'll add it
|
|
||
| @Override | ||
| public void close() throws IOException { | ||
| if (rowRules != null) { |
There was a problem hiding this comment.
didn't get this. Why do we do this at closing
There was a problem hiding this comment.
rowRules is used to limit the number of processed data by engine, we need to ensure that the number of processed data is within the range of [min_row, max_row]. In write method, we add counter the after handler a row and compare it in close method finally.
There was a problem hiding this comment.
so the row rule must be a single parallelism right, looks like this counter only count the number of rows in this writer.
There was a problem hiding this comment.
thanks for pointing this out, i think so, if we want to limit row number, the parallelism must be one, can we set it writer_parallelism_num by this conf, or any suggestions, thx
There was a problem hiding this comment.
if the assert sink only support single parallelism, we should make sure the single parallelism was hard coded to ensure the correct result. The related class is ParallelismComputable.
If we wanna support multi parallelism, I believe we should some how aggregate the count in the coordinator. We could make the multi parallelism support as a follow up task for future work.
There was a problem hiding this comment.
Thanks so much, got it, i'll look ParallelismComputable first.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public class AssertRuleParser implements Serializable { |
There was a problem hiding this comment.
IMO UT is necessary for this class. can we add it please
There was a problem hiding this comment.
sure, by the way, what's the meaning of IMO? :)
There was a problem hiding this comment.
it stands for in my opinion :)
|
@garyli1019 The ci failed, and error message shows failing to download maven dependency, which may be caused by network problems.
|
There was a problem hiding this comment.
LGTM~ Thanks for your contribution @liuxiaocs7 , would you please make a follow up PR to add the documentation about this assert sink?
sure, I'd love to do |
Signed-off-by:
Pre-Checklist
Note: Please complete ALL items in the following checklist.
Purpose
Some description about what this PR wants to do.
Approaches
Currently we support print sink and we use print in some tests, but print is not sufficient enough to verify the test result.
So we implement an AssertSink to verify the test result more accurately.
Related Issues
Close #153
New Behavior (screenshots if needed)
N/A