BuxOrg / bux

Bitcoin UTXO & xPub Management Engine

Apache License 2.0

36 stars 6 forks source link

Pull Request Checklist

[x] 📖 I created my PR using provided : CODE_STANDARDS
[x] 📖 I have read the short Code of Conduct: CODE_OF_CONDUCT
[x] 🏠 I tested my changes locally.
[x] ✅ I have provided tests for my changes.
[x] 📝 I have used conventional commits.
[ ] 📗 I have updated any related documentation.
[x] 💾 PR was issued based on the Github or Jira issue.

Initially, the goal of this task was to simplify the taskmanager by fixing taskq and localCron and make some refactorization & simplification changes. All are done:

I renamed all taskmanager.client-related names/filenames to more precise ones;
Removed unnecessary taskmanager.With functional options;
Simplified Redis configuration: taskmanager.WithRedis function;
Removed our localCron structure, instead, the github.com/robfig/cron/v3 - cronService is used directly.

After my changes, a lot of test files had to be adjusted - that's why so many files are modified.

I tested it locally running different examples and bux-server with backend/frontend. Tasks were correctly scheduled.

TL;DR

Additionally, I tested the bux, running several parallel instances with Redis configuration for taskq. Apparently, the codebase before my changes didn't allow to run it that way. So I fixed the issue, according to task-doc, adding:

if factoryType == FactoryRedis {
  if err := q.Consumer().Start(ctx); err != nil {
      return err
  }
}

After that, the taskq in different nodes (instances) started to communicate via Redis.

But another problem appeared. Number of cronJob executions in defined period was about number of active nodes.

For example, For cronJob with 30sec period and 5 active instances: The job executions among all nodes was 5.

In this PR I'm proposing a fix using distributed redis locks with TTL.

Every node has local cron enabled.

At the beginning of a cron handler I try to lock the task like this:

tryLock = func() bool {
boolCmd := c.options.taskq.config.Redis.SetNX(ctx, key, "1", lockTime)
return boolCmd.Val()
}

If any node didn't execute the job in lockTime period, the job is added to ataskq queue.

Diagrams for better understanding:

Previous solution

New solution

I tested it with 6 instances and collecting local metrics. My example task has 2sec period and every time it was executed it sent 1(one) to my local influxDB.

I analysed the results, grouping metrics in 2sec aggregation window and then summing the number of points per window.

Without cronlocks

We can see, that the number of tasks executed per period is not constant and definitely it is not one.

With cronlocks

Regardless how many instances are active, only one cronJob handler is executed per time window.

Note:

Of course, the solution can be developed, but, for now:

it doesn't break the single-node architecture
generally it allows to run app in multi-node way

In the future we should re-think whether using taskq is a good idea for us:

It uses obsolete 'redis/v8' library
It is not considered as production ready (https://github.com/vmihailenco/taskq/issues/181)

Maybe a good alternative will be asynq.

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (1d3bbb2) 53.18% compared to head (12a41f8) 53.04%. Report is 8 commits behind head on master.

Additional details and impacted files

[![Impacted file tree graph](https://app.codecov.io/gh/BuxOrg/bux/pull/510/graphs/tree.svg?width=650&height=150&src=pr&token=gh8XH5lven&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg)](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg) ```diff @@ Coverage Diff @@ ## master #510 +/- ## ========================================== - Coverage 53.18% 53.04% -0.15% ========================================== Files 110 107 -3 Lines 11251 11146 -105 ========================================== - Hits 5984 5912 -72 + Misses 4815 4787 -28 + Partials 452 447 -5 ``` | [Flag](https://app.codecov.io/gh/BuxOrg/bux/pull/510/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/BuxOrg/bux/pull/510/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg) | `53.04% <75.14%> (-0.15%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg) | Coverage Δ | | |---|---|---| | [client.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-Y2xpZW50Lmdv) | `60.50% <100.00%> (-0.59%)` | :arrow_down: | | [client\_internal.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-Y2xpZW50X2ludGVybmFsLmdv) | `72.04% <100.00%> (ø)` | | | [client\_options.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-Y2xpZW50X29wdGlvbnMuZ28=) | `72.85% <100.00%> (-0.46%)` | :arrow_down: | | [taskmanager/options.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-dGFza21hbmFnZXIvb3B0aW9ucy5nbw==) | `100.00% <100.00%> (ø)` | | | [taskmanager/cron\_jobs.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-dGFza21hbmFnZXIvY3Jvbl9qb2JzLmdv) | `51.85% <57.14%> (-4.82%)` | :arrow_down: | | [taskmanager/taskmanager.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-dGFza21hbmFnZXIvdGFza21hbmFnZXIuZ28=) | `70.76% <70.76%> (ø)` | | | [taskmanager/taskq.go](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg#diff-dGFza21hbmFnZXIvdGFza3EuZ28=) | `77.19% <73.17%> (+7.28%)` | :arrow_up: | ------ [Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg). Last update [1d3bbb2...12a41f8](https://app.codecov.io/gh/BuxOrg/bux/pull/510?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BuxOrg).