Delete and Append refresh apis should not throw error if data isn't changed #189
Conversation
|
|
||
| if (deletedFiles.isEmpty) { | ||
| throw NoChangesDetected("Refresh aborted as no deleted source data file found.") | ||
| } | ||
|
|
There was a problem hiding this comment.
for reviewers: this order is changed because if there are no deleted files, we don't really need to go further and fail the operation for lineage column missing.
apoorvedave1
changed the title
|
I think it would be good to check the condition - no data change - for "full" refresh mode? |
Fair enough, created this issue #190 |
src/main/scala/com/microsoft/hyperspace/HyperspaceException.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/microsoft/hyperspace/actions/RefreshAppendAction.scala
Outdated
Show resolved
Hide resolved
|
@apoorvedave1 Can you check the build failure? |
src/main/scala/com/microsoft/hyperspace/actions/NoChangesException.scala
Outdated
Show resolved
Hide resolved
| logWarning(e.msg) | ||
| case e: Exception => | ||
| logEvent(event(appInfo, message = s"Operation Failed: ${e.getMessage}.")) | ||
| logEvent(event(appInfo, message = s"Operation failed: ${e.getMessage}")) |
There was a problem hiding this comment.
for reviewers: the trailing full stop "." is removed in the lines after ${e.getMessage} because there are cases when the e.getMessage contains a . (refer this for example)
src/test/scala/com/microsoft/hyperspace/index/IndexManagerTests.scala
Outdated
Show resolved
Hide resolved
imback82
left a comment
imback82
left a comment
There was a problem hiding this comment.
Few minor comments, but LGTM, thanks @apoorvedave1!
src/test/scala/com/microsoft/hyperspace/index/IndexManagerTests.scala
Outdated
Show resolved
Hide resolved
Co-authored-by: Terry Kim <yuminkim@gmail.com>
Co-authored-by: Terry Kim <yuminkim@gmail.com>
…into errorhandling
| protected lazy val spark: SparkSession = SparkSession | ||
| .builder() | ||
| .master(s"local[$numParallelism]") | ||
| .config(HYPERSPACE_EVENT_LOGGER_CLASS_KEY, "com.microsoft.hyperspace.MockEventLogger") |
There was a problem hiding this comment.
for reviewers: setting this config at the beginning of tests because it's a lazy val in a object EventLogger class, and hence it is initialized only once at the beginning of the test suite.
I tried setting this within a test but since the lazy val got initialized before this config could take effect, it did not work.
imback82
left a comment
imback82
left a comment
There was a problem hiding this comment.
LGTM, thanks @apoorvedave1!
3 participants
What is the context for this pull request?
Hyperspace throws error for
refreshIndexapi for following cases:Ideally the behavior should be, a
refreshIndexjust end operation saying "Nothing to do!!".This PR introduces another exception type:
NoChangesDetected. The baseActionclass eats up these exception with logging what happened and why it's not necessary to continue this operation.The end user just sees log warnings but their code is not broken if source data is not modified.
What changes were proposed in this pull request?
Change the behavior of refresh api for deletes and appends if source data is not modified.
Does this PR introduce any user-facing change?
Yes:
Hyperspace will NOT throw error for
refreshIndexapi for following cases:How was this patch tested?
updated unit tests for this case