Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-21872 Old index values returned after index is overwritten #12520

Merged
merged 1 commit into from May 14, 2019

Conversation

richardkchapman
Copy link
Member

Clear key cache for any index file that is written by hthor.

For roxie, clear entire index key cache when a file is written (since the id's
stored in the cache do not seem to correspond to physical filenames, it's
non-trivial to clear just the one that is written.

Signed-off-by: Richard Chapman rchapman@hpccsystems.com

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

using the test file in the Jira, which I added to the test suite.

@richardkchapman
Copy link
Member Author

@jakesmith Please review

@AttilaVamos
Copy link
Contributor

@richardkchapman there is a regression error (same on all engines) in this PR :

322. Test: indexcachebug.ecl
    --- indexcachebug.xml
    +++ indexcachebug.xml
    @@ -1,3 +1,9 @@
    +<Dataset name='Result 1'>
    +</Dataset>
    +<Dataset name='Result 2'>
    +</Dataset>
    +<Dataset name='Result 3'>
    +</Dataset>
     <Dataset name='Result 4'>
      <Row><fname>Aaron          </fname><lname>Jones          </lname><age>100</age></Row>
      <Row><fname>Adam           </fname><lname>Smith          </lname><age>90</age></Row>
    @@ -9,6 +15,8 @@
      <Row><fname>Edward         </fname><lname>Green          </lname><age>30</age></Row>
      <Row><fname>Egbert         </fname><lname>Sillyname      </lname><age>20</age></Row>
      <Row><fname>Freddy         </fname><lname>Peters         </lname><age>10</age></Row>
    +</Dataset>
    +<Dataset name='Result 5'>
     </Dataset>
     <Dataset name='Result 6'>
      <Row><fname>1              </fname><lname>4280804635     </lname><age>999</age></Row>

Somehow the Smoketest failed to put result report into GitHub. I'm investigating it.

@AttilaVamos
Copy link
Contributor

Hmm, by the log:"

{
  "message": "Validation Failed",
  "errors": [
    {
      "resource": "IssueComment",
      "code": "unprocessable",
      "field": "data",
      "message": "User is blocked"
    }
  ],
  "documentation_url": "https://developer.github.com/v3/issues/comments/#create-a-comment"
}

It is strange, based on other test session (PR-12521) after this, finished and reported well.

@richardkchapman
Copy link
Member Author

richardkchapman commented May 8, 2019

I pushed a fix

@AttilaVamos
Copy link
Contributor

I'm watching.

@AttilaVamos
Copy link
Contributor

Still strange. Smoketest wasn't able to add test session started comment to this PR.

@richardkchapman
Copy link
Member Author

I wonder if it's because I blocked the SmokeTest user (I thought that would stop me getting notifications, but maybe it also stops smoketest from being able to comment on my PRs)

@richardkchapman
Copy link
Member Author

I suspect that was it (having read the docs more carefully).

I have unblocked - may also need to re-add as a collaborator. Let me know if you spot anything else I messed up.

@AttilaVamos
Copy link
Contributor

Yes, it seems. I tried to add a comment via browser as a Smoketest user and I wasn't able to do that.

@HPCCSmoketest
Copy link
Contributor

Test comment

@AttilaVamos
Copy link
Contributor

It seems ok now for Smoketest user.
There is still an error (Hthor, Thor):

    322. Test: indexcachebug.ecl
    --- indexcachebug.xml
    +++ indexcachebug.xml
    @@ -1,5 +1,3 @@
    -
    -<Result>
     <Dataset name='Result 1'>
     </Dataset>
     <Dataset name='Result 2'>
    @@ -32,4 +30,3 @@
      <Row><fname>8              </fname><lname>2548245530     </lname><age>992</age></Row>
      <Row><fname>9              </fname><lname>704836619      </lname><age>991</age></Row>
     </Dataset>
    -</Result>

Roxie:

337. Test: indexcachebug.ecl
    --- indexcachebug.xml
    +++ indexcachebug.xml
    @@ -1,5 +1,5 @@
    -
    -<Result>
    +<Warning><Code>4523</Code><Filename>indexcachebug.ecl</Filename><Line>29</Line><Source>eclcc</Source><Message>Neither LIMIT() nor CHOOSEN() supplied for index read on  '~myindex'</Message></Warning>
    +<Warning><Code>4523</Code><Filename>indexcachebug.ecl</Filename><Line>29</Line><Source>eclcc</Source><Message>Neither LIMIT() nor CHOOSEN() supplied for index read on  '~myindex'</Message></Warning>
     <Dataset name='Result 1'>
     </Dataset>
     <Dataset name='Result 2'>
    @@ -32,4 +32,3 @@
      <Row><fname>8              </fname><lname>2548245530     </lname><age>992</age></Row>
      <Row><fname>9              </fname><lname>704836619      </lname><age>991</age></Row>
     </Dataset>
    -</Result>

Clear key cache for any index file that is written by hthor.

For roxie, clear entire index key cache when a file is written (since the id's
stored in the cache do not seem to correspond to physical filenames, it's
non-trivial to clear just the one that is written.

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>
@richardkchapman
Copy link
Member Author

I force-pushed another change.

@HPCCSmoketest
Copy link
Contributor

Automated Smoketest: ✅
OS: centos 7.4.1708 (Linux 3.10.0-327.28.3.el7.x86_64)
Sha: ca1b812
Build: success
Build: success
Install HPCC Platform
HPCC Start: OK

Unit tests result:

Test total passed failed errors timeout elaps
unittest 113 113 0 0 0 36 sec
wutoolTest(Dali) 19 19 0 0 0 2 sec
wutoolTest(Cassandra) 19 19 0 0 0 7 sec

Regression test result:

phase total pass fail elaps
setup (hthor) 11 11 0 24 sec (00:00:24)
setup (thor) 11 11 0 44 sec (00:00:44)
setup (roxie) 11 11 0 20 sec (00:00:20)
test (hthor) 829 829 0 164 sec (00:02:44)
test (thor) 754 754 0 602 sec (00:10:02)
test (roxie) 905 905 0 218 sec (00:03:38)

HPCC Stop: OK
Time stats:

Prep time Build time Package time Install time Start time Test time Stop time Summary
29 sec (00:00:29) 211 sec (00:03:31) 0 sec (00:00:00) 3 sec (00:00:03) 18 sec (00:00:18) 1313 sec (00:21:53) 21 sec (00:00:21) 1595 sec (00:26:35)

@AttilaVamos
Copy link
Contributor

I don't know where is the Smoketest report comment (by the logs it is created successfully), but this PR is clean now.

@richardkchapman
Copy link
Member Author

@jakesmith Please review

@@ -12290,6 +12290,7 @@ class CRoxieServerIndexWriteActivity : public CRoxieServerInternalSinkActivity,
duplicateKeyCount = builder->getDuplicateCount();
cummulativeDuplicateKeyCount += duplicateKeyCount;
builder->finish(metadata, &fileCrc);
clearKeyStoreCache(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this isn't specifically clearing the file just written (as in the hthor change) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I address this in the commit comment:

For roxie, clear entire index key cache when a file is written (since the id's
stored in the cache do not seem to correspond to physical filenames, it's
non-trivial to clear just the one that is written.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok, missed that.

@jakesmith
Copy link
Member

@richardkchapman - one question

@richardkchapman
Copy link
Member Author

@jakesmith See answer

@jakesmith
Copy link
Member

@richardkchapman - good to merge.

@richardkchapman
Copy link
Member Author

@ghalliday Please merge

@ghalliday ghalliday merged commit 18a7e05 into hpcc-systems:candidate-7.2.x May 14, 2019
@richardkchapman richardkchapman deleted the hpcc21872 branch February 15, 2020 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants