Skip to content

Wait for rules to be refreshed in BaseCaseInsensitiveMappingTest#11506

Merged
hashhar merged 1 commit intotrinodb:masterfrom
hashhar:hashhar/sleep-in-test
Mar 17, 2022
Merged

Wait for rules to be refreshed in BaseCaseInsensitiveMappingTest#11506
hashhar merged 1 commit intotrinodb:masterfrom
hashhar:hashhar/sleep-in-test

Conversation

@hashhar
Copy link
Copy Markdown
Member

@hashhar hashhar commented Mar 16, 2022

A refresh period of 1ms doesn't guarantee that rules will be refreshed
before they are used. Since the rules are refreshed synchronously we
sleep for 2*refresh period to guarantee that rules are refreshed.

Documentation

(x) No documentation is needed.

Release notes

(x) No release notes entries required.

@hashhar hashhar added the no-release-notes This pull request does not require release notes entry label Mar 16, 2022
@hashhar hashhar requested review from findepi and kokosing March 16, 2022 10:00
@cla-bot cla-bot bot added the cla-signed label Mar 16, 2022
Comment on lines 361 to 362
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2* isn't necessary, is it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To be safe. should be refreshPeriod + some delta

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Kept 2x, moved sleep to the Utils class.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be refreshPeriod + some delta

i don't believe it should

To be safe.

fine, keep it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Even if you introduce a new method, it could be called updateRuleBasedIdentifierMappingFile (without "AndWait")

  1. smaller diff
  2. it makes it much harder to invoke the old method directly, since it's shadowed

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved sleep to existing method. Thanks.

@hashhar hashhar force-pushed the hashhar/sleep-in-test branch from b33f6b6 to e6b2bba Compare March 16, 2022 10:11
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

% comment. Thanks!

@hashhar hashhar force-pushed the hashhar/sleep-in-test branch from e6b2bba to 68c1b8d Compare March 16, 2022 12:36
@hashhar hashhar requested a review from findepi March 16, 2022 12:37
A refresh period of 1ms doesn't guarantee that rules will be refreshed
before they are used. Since the rules are refreshed synchronously we
sleep for 2*refresh period to guarantee that rules are refreshed.
@hashhar hashhar force-pushed the hashhar/sleep-in-test branch from 68c1b8d to 8d818e7 Compare March 16, 2022 12:39
@hashhar hashhar merged commit c9ad71f into trinodb:master Mar 17, 2022
@hashhar hashhar deleted the hashhar/sleep-in-test branch March 17, 2022 21:01
@github-actions github-actions bot added this to the 375 milestone Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

3 participants