[SPARK-29999][SS][FOLLOWUP] Fix test to check the actual metadata log directory #28930
Conversation
|
cc. @gengliangwang @cloud-fan @HyukjinKwon as they've reviewed #26671 |
|
I am not very familiar with streaming but LGTM |
|
Ah that's OK. Thanks for reviewing :) cc @dongjoon-hyun @zsxwing and @gatorsmile from #26639 (original PR of SPARK-29999) |
|
Test build #124532 has finished for PR 28930 at commit
|
|
|
||
| def getMetadataLogPath(path: Path, hadoopConf: Configuration, sqlConf: SQLConf): Path = { | ||
| val metadataDir = new Path(path, FileStreamSink.metadataDir) | ||
| val fs = metadataDir.getFileSystem(hadoopConf) |
There was a problem hiding this comment.
shall we pass the fs as a parameter as well?
| def getMetadataLogPath(path: Path, hadoopConf: Configuration, sqlConf: SQLConf): Path = { | ||
| val metadataDir = new Path(path, FileStreamSink.metadataDir) | ||
| val fs = metadataDir.getFileSystem(hadoopConf) | ||
| FileStreamSink.checkEscapedMetadataPath(fs, metadataDir, sqlConf) |
There was a problem hiding this comment.
this is new code. So this PR fixes a bug instead of just fixing a test?
There was a problem hiding this comment.
Oh sorry I should have explained before. That's to avoid test to copy same code (logic) what FileStreamSink does, so that we don't break the test when we somehow change it. So that's a refactor and doesn't mean there's a bug in FileStreamSink.
|
retest this please |
|
Test build #124629 has finished for PR 28930 at commit
|
|
thanks, merging to master/3.0! |
… directory ### What changes were proposed in this pull request? This patch fixes the missed spot - the test initializes FileStreamSinkLog with its "output" directory instead of "metadata" directory, hence the verification against sink log was no-op. ### Why are the changes needed? Without the fix, the verification against sink log was no-op. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Checked with debugger in test, and verified `allFiles()` returns non-zero entries. (It returned zero entry, as there's no metadata.) Closes #28930 from HeartSaVioR/SPARK-29999-FOLLOWUP-fix-test. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 5472170) Signed-off-by: Wenchen Fan <[email protected]>
|
Thanks all for reviewing and merging! |
4 participants
What changes were proposed in this pull request?
This patch fixes the missed spot - the test initializes FileStreamSinkLog with its "output" directory instead of "metadata" directory, hence the verification against sink log was no-op.
Why are the changes needed?
Without the fix, the verification against sink log was no-op.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Checked with debugger in test, and verified
allFiles()returns non-zero entries. (It returned zero entry, as there's no metadata.)