Disable indexes with non-empty "appended" or "deleted" files if hybrid-scan is disabled #194

apoorvedave1 · 2020-10-09T23:56:16Z

What is the context for this pull request?

Fixes #193

Tracking Issue: Hyperspace should not pick indexes if hybrid scan is disabled and "appendedFiles" or "deletedFiles" in metadata is non-empty #193
Parent Issue: Merge append and delete actions on indexes on modified source data #187

What changes were proposed in this pull request?

Remove any index candidate for being picked when:

hybrid scan is disabled AND
either deletedFiles or appendedFiles in index metadata is non-empty

Does this PR introduce any user-facing change?

no. It's a bug fix.

How was this patch tested?

added unit tests

…into errorhandling

…into bugfixes

…into bugfixes # Conflicts: # src/test/scala/com/microsoft/hyperspace/TestUtils.scala

…into bugfixes

Co-authored-by: Terry Kim <yuminkim@gmail.com>

…into errorhandling

…into bugfixes

# Conflicts: # src/main/scala/com/microsoft/hyperspace/actions/RefreshDeleteAction.scala # src/test/scala/com/microsoft/hyperspace/index/RefreshIndexTests.scala

sezruby · 2020-10-10T02:43:59Z

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala

+      indexes.filter(
+        index =>
+          index.created && signatureValid(index) &&
+            index.deletedFiles.isEmpty && index.appendedFiles.isEmpty)


This would be ok but might cause some bad user experience:

refreshIncremental

failed to apply index

no idea or clue for the failure until the user checks index log entry ..

Thanks @sezruby for bringing this up, i think that's ok. This behavior is expected. I think this experience is exactly same as the following:

user creates index

user updates source data

fail to apply index

no idea or clue for the failure until the user checks index log entry ..

Please feel free to suggest how to improve this experience. We can create an issue for the same and fix it in subsequent PRs.

On the other hand, if we don't do this, it will lead to incorrect results.

The user experience currently is not good either. For example, with hybrid scan off, if you add a file, it will fail in signatureValid without letting the user know. We have been talking about exposing "why not" API that tells the user why an index was not picked up. Until we introduce that kind of API, I think the user experience will remain "not desirable" - meaning not good. 😄

I think @apoorvedave1 beat me on this comment 😄

@imback82 @apoorvedave1
This is why I suggested quick failure (assert) rather than storing deletedFiles but maybe we could hybrid scan with it later.

Now we are proposing "mutable" dataset, I think we need to address this issue with more care.

Anyway I'm fine with the current approach as it's also valid :)

Thanks all!

OK, in this case, I still don't understand 😄. @rapoth do you understand this scenario?

Ok let me try again 😁

disable "global" hybrid scan config

user can choose some "beneficial - with high perf improvement" indexes with little diff and do quick refresh for the indexes

hybrid scan can be performed for quick refreshed indexes even if the global config is disabled. The other unrefreshed indexes with diff won't be the candidates for hybrid scan.

Thanks 👍 . So in a nutshell, the selective hybrid scan is "we can apply the hybrid scan even if the user didn't enable it only when the signature matches", right?

Right. That's the point 👍

…bug_193

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala

imback82 · 2020-10-10T05:07:14Z

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala

+      indexes.filter(
+        index =>
+          index.created && signatureValid(index) &&
+            index.deletedFiles.isEmpty && index.appendedFiles.isEmpty)


The user experience currently is not good either. For example, with hybrid scan off, if you add a file, it will fail in signatureValid without letting the user know. We have been talking about exposing "why not" API that tells the user why an index was not picked up. Until we introduce that kind of API, I think the user experience will remain "not desirable" - meaning not good. 😄

imback82

Few minor comments, but LGTM, thanks @apoorvedave1!

src/test/scala/com/microsoft/hyperspace/index/rules/HyperspaceRuleTestSuite.scala

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala

imback82 · 2020-10-10T07:35:16Z

@apoorvedave1 Can you help creating an issue to address improving the user experience when indexes are not selected (e.g., the "why not" API)? I think we should address this sooner than later (a good candidate for the next release - 0.5). Thanks!

rapoth · 2020-10-12T02:40:34Z

@apoorvedave1 Can you help creating an issue to address improving the user experience when indexes are not selected (e.g., the "why not" API)? I think we should address this sooner than later (a good candidate for the next release - 0.5). Thanks!

@apoorvedave1 Can you take care of creating this issue with the relevant details?

apoorvedave1 and others added 30 commits October 7, 2020 20:39

initial commit

b0771bf

Merge branch 'master' into errorhandling

bd89f91

test name fix

37c0dc8

Update RefreshIndexTests.scala

739d474

initial commit part 2

ad2e9df

review comments

4d27fe2

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

58aea9d

…into errorhandling

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

1c4ae1c

…into bugfixes

doc

87ff7b7

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

300703b

…into bugfixes

build failure fix

e31f171

add unit test for "appended"files update

47c6730

test fix

7b4a6a8

review comments

f8766c0

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

5a171f9

…into bugfixes # Conflicts: # src/test/scala/com/microsoft/hyperspace/TestUtils.scala

set logger at session initialization in test cases

58f1339

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

4dc0ab6

…into bugfixes

move mock logger setting to beginning of all SparkInvolvedSuite tests

2c55d86

refactoring

138e96a

Co-authored-by: Terry Kim <yuminkim@gmail.com>

explicit assert in test

767513f

Co-authored-by: Terry Kim <yuminkim@gmail.com>

review comets

7338de5

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

7f94a1d

…into errorhandling

test bug fixes

29ca9b3

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

5db1125

…into bugfixes

whitespace fix

9d26ddc

Merge branch 'errorhandling' of github.com:apoorvedave1/hyperspace-1 …

c7816b6

…into bugfixes

add refresh-append test for updating delted files

b43b29e

Merge remote-tracking branch 'upstream/master' into bugfixes

7ebda9a

# Conflicts: # src/main/scala/com/microsoft/hyperspace/actions/RefreshDeleteAction.scala # src/test/scala/com/microsoft/hyperspace/index/RefreshIndexTests.scala

Merge branch 'master' into bugfixes

abbc59f

bug fix in updating index log entry after refresh append and deleted

8f55e66

apoorvedave1 requested review from imback82, pirz, rapoth and sezruby October 10, 2020 00:10

apoorvedave1 self-assigned this Oct 10, 2020

apoorvedave1 added 2 commits October 9, 2020 19:24

review comments

f5927ac

rename test path

521edcb

sezruby reviewed Oct 10, 2020

View reviewed changes

apoorvedave1 added 4 commits October 9, 2020 19:44

Merge branch 'bugfixes' of github.com:apoorvedave1/hyperspace-1 into …

97099ef

…bug_193

add test for "deletedFiles" in entry

2080edb

unit tests added

3af06eb

Merge remote-tracking branch 'upstream/master' into bug_193

607ae9e

apoorvedave1 marked this pull request as ready for review October 10, 2020 04:24

apoorvedave1 requested a review from sezruby October 10, 2020 04:50

apoorvedave1 added this to the 0.4.0 milestone Oct 10, 2020

imback82 reviewed Oct 10, 2020

View reviewed changes

apoorvedave1 added 3 commits October 9, 2020 23:35

review comments, test fixes

315b36d

rephrase test name

42de97a

autoformat result

6d1f0fc

apoorvedave1 requested a review from imback82 October 10, 2020 06:53

apoorvedave1 mentioned this pull request Oct 10, 2020

Merge append and delete actions on indexes on modified source data #187

Merged

imback82 reviewed Oct 10, 2020

View reviewed changes

src/test/scala/com/microsoft/hyperspace/index/rules/HyperspaceRuleTestSuite.scala Show resolved Hide resolved

src/test/scala/com/microsoft/hyperspace/index/rules/RuleUtilsTest.scala Outdated Show resolved Hide resolved

review comments

03f23da

apoorvedave1 requested a review from imback82 October 10, 2020 07:21

imback82 approved these changes Oct 10, 2020

View reviewed changes

imback82 merged commit 0de4734 into microsoft:master Oct 10, 2020

apoorvedave1 deleted the bug_193 branch October 12, 2020 01:24

sezruby mentioned this pull request Oct 12, 2020

Revisit index metadata for mutable dataset #198

Closed

Disable indexes with non-empty "appended" or "deleted" files if hybrid-scan is disabled #194

Disable indexes with non-empty "appended" or "deleted" files if hybrid-scan is disabled #194

Uh oh!

Conversation

apoorvedave1 commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the context for this pull request?

What changes were proposed in this pull request?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

imback82 commented Oct 10, 2020

Uh oh!

rapoth commented Oct 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apoorvedave1 commented Oct 9, 2020 •

edited

Loading