search

xcherryio / xcherry

server and main repo of xCherry project
Apache License 2.0
26 stars 1 forks source link

Reduce sql operations #68

Closed zklgame closed 10 months ago

zklgame commented 10 months ago

Why make this pull request?

Improve the logic of handling local queue messages and make exactly-once update to each table row.

What has changed

[Summarize what components of the repo is updated]

[Link to xdb-apis/xdb-golang-sdk PRs if it's on top of any API changes]

How to test this pull request?

[If writing Integration test in Golang SDK repo, please provide link to the pull request of Golang SDK Repo]

Checklist before merge

[ ] If applicable, merge the xdb-apis/xdb-golang-sdk PRs to main branch [ ] If applicable, merge the xdb-apis/xdb-apis PRs to main branch [ ] Update go.mod to use the commitID of the main branches for xdb-apis/xdb-golang-sdk

codecov[bot] commented 10 months ago

Codecov Report

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

Comparison is base (1c7a044) 63.11% compared to head (42e94ae) 62.96%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #68 +/- ## ========================================== - Coverage 63.11% 62.96% -0.15% ========================================== Files 62 62 Lines 4620 4631 +11 ========================================== Hits 2916 2916 - Misses 1511 1522 +11 Partials 193 193 ``` | [Files](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab) | Coverage Δ | | |---|---|---| | [extensions/postgres/transactional.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-ZXh0ZW5zaW9ucy9wb3N0Z3Jlcy90cmFuc2FjdGlvbmFsLmdv) | `57.79% <ø> (ø)` | | | [persistence/data\_models.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-cGVyc2lzdGVuY2UvZGF0YV9tb2RlbHMuZ28=) | `25.00% <ø> (ø)` | | | [persistence/sql/start\_process.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-cGVyc2lzdGVuY2Uvc3FsL3N0YXJ0X3Byb2Nlc3MuZ28=) | `57.51% <100.00%> (ø)` | | | [engine/immediate\_task\_conrrent\_processor.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-ZW5naW5lL2ltbWVkaWF0ZV90YXNrX2NvbnJyZW50X3Byb2Nlc3Nvci5nbw==) | `81.67% <0.00%> (+0.50%)` | :arrow_up: | | [persistence/data\_models\_json\_fields.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-cGVyc2lzdGVuY2UvZGF0YV9tb2RlbHNfanNvbl9maWVsZHMuZ28=) | `91.25% <55.55%> (ø)` | | | [persistence/sql/process\_local\_queue.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-cGVyc2lzdGVuY2Uvc3FsL3Byb2Nlc3NfbG9jYWxfcXVldWUuZ28=) | `0.00% <0.00%> (ø)` | | | [persistence/sql/sql\_process\_store\_common.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-cGVyc2lzdGVuY2Uvc3FsL3NxbF9wcm9jZXNzX3N0b3JlX2NvbW1vbi5nbw==) | `27.63% <0.00%> (-17.05%)` | :arrow_down: | | [persistence/sql/wait\_until.go](https://app.codecov.io/gh/xdblab/xdb/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xdblab#diff-cGVyc2lzdGVuY2Uvc3FsL3dhaXRfdW50aWwuZ28=) | `24.84% <1.69%> (-0.49%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zklgame commented 10 months ago

This refactor uses a single doProcessLocalQueueCommandsAndMessagesTx method to handle the logic for both updateWaitUntil and processLocalQueueMessages. Compared to the proposal in https://github.com/xdblab/xdb/pull/63#pullrequestreview-1697935884, the pros and cons of this refactor are:

In terms of functionality, these two methods can achieve the same results and performance because they both update the related rows in tables only once.

@longquanzheng What's your preference now?

longquanzheng commented 10 months ago

This refactor uses a single doProcessLocalQueueCommandsAndMessagesTx method to handle the logic for both updateWaitUntil and processLocalQueueMessages. Compared to the proposal in #63 (review), the pros and cons of this refactor are:

  • pros: highly resuse the codes and reduce maintainance effort for related logic.
  • cons: might not as straightforward as the other one and sacrifice some readability.

In terms of functionality, these two methods can achieve the same results and performance because they both update the related rows in tables only once.

@longquanzheng What's your preference now?

Yeah, it's indeed a better/improved version than your previous one, and technically the same perf as what I proposed. I can clearly see the more code reuse than what I proposed:

And as you pointed out the readability:

Finally for tradeoff in my own opinion, these additional code reuse with this is not as valuable as readability and extensibility.