Skip to content

[Branch-2.7] Fix ledger roll over scheduler task#11226

Merged
codelipenghui merged 2 commits into
apache:branch-2.7from
gaoran10:fix-ledger-roll-over-scheduler-task
Jul 9, 2021
Merged

[Branch-2.7] Fix ledger roll over scheduler task#11226
codelipenghui merged 2 commits into
apache:branch-2.7from
gaoran10:fix-ledger-roll-over-scheduler-task

Conversation

@gaoran10

@gaoran10 gaoran10 commented Jul 6, 2021

Copy link
Copy Markdown
Contributor

The PR #11116 couldn't be cherry-picked to branch-2.7, because there are too many conflicts.

In PR #8946 the ledger rollover task had been moved from BrokerService to ManagedLedgerImpl, and this PR is based on it.

Motivation

Currently, the ledger rollover scheduled task will execute before reach the ledger maximum rollover time, this will cause the ledger doesn't roll over in time.

Modifications

Only make the ledger rollover scheduled task after the ledger created successfully.
If the scheduled task was executed when there is no entry in the current ledger, the scheduled task will not be re-executed, and if there is new entry is added the ledger will rollover.

Verifying this change

Add a unit test to verify the ledger could be rolled over in time.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

@gaoran10 gaoran10 changed the title [Broker] Fix ledger roll over scheduler task [Branch-2.7] Fix ledger roll over scheduler task Jul 6, 2021
@gaoran10 gaoran10 self-assigned this Jul 6, 2021

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

overall the change looks good to me.

but I am afraid that we are adding a new flaky test

can you please check my comment ?

// the ledger rollover scheduled time is between 1000 and 1050 ms,
// wait 1100 ms, the ledger should be rolled over.
Awaitility.await()
.atMost(1100, TimeUnit.MILLISECONDS)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure that on CI we can rely on making the difference in the order of 100ms

it is possible that the machine is very slow and then we will see the test failing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll increase the time order of magnitude, we could set the maximum rollover time to 10 seconds and wait 11 seconds? Maybe 1 second is more stable?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's try

@lhotari what do you think ?

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.

In general, I think it's better not to test accurate timings. The test code is cleaner when .atMost is omitted. The default value is 10 seconds and that should be suitable for most cases where the operation is expected to complete in less than 10 seconds.

@gaoran10
gaoran10 force-pushed the fix-ledger-roll-over-scheduler-task branch from 982fb46 to 71c86d3 Compare July 8, 2021 06:22
gaoran10 added 2 commits July 8, 2021 21:44
…edLedgerImpl`

2. scheduled the ledger rollover task after update ledger last created time
@gaoran10
gaoran10 force-pushed the fix-ledger-roll-over-scheduler-task branch from 71c86d3 to 9b02015 Compare July 8, 2021 13:44
@codelipenghui
codelipenghui merged commit 732395f into apache:branch-2.7 Jul 9, 2021
@gaoran10
gaoran10 deleted the fix-ledger-roll-over-scheduler-task branch July 9, 2021 01:37
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants