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

Refactor GetNextResourceIndex to make it work properly with transaction #21469

Merged
merged 7 commits into from
Oct 16, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Oct 15, 2022

Related:

This PR uses a general and stable method to generate resource index (eg: Issue Index, PR Index)

If the code looks good, I can add more tests

ps: please skip the diff, only have a look at the new code. It's entirely re-written.

@6543 6543 added this to the 1.18.0 milestone Oct 15, 2022
@wxiaoguang wxiaoguang force-pushed the fix-res-index branch 2 times, most recently from a7a0756 to a15c8f5 Compare October 15, 2022 15:30
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Code-wise looks good. I'll trust you on the write-lock SQL code, no expertise on that.

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Oct 15, 2022
@Gusted Gusted added the type/enhancement An improvement of existing functionality label Oct 15, 2022
@wxiaoguang
Copy link
Contributor Author

Added tests.

Ready for review & merge.

models/db/index.go Outdated Show resolved Hide resolved
models/db/index.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 16, 2022
@lunny
Copy link
Member

lunny commented Oct 16, 2022

Maybe we need to ensure SyncMaxResourceIndex or GetNextResourceIndex is in a transaction.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Oct 16, 2022

Maybe we need to ensure SyncMaxResourceIndex or GetNextResourceIndex is in a transaction.

Agree. Although the current approach also works without transaction (if there is no high concurrency access ...), I hope there is a mechanism to detect whether the context has an open transaction. If there is such mechanism, we can update the code to add the checks.

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2022

Codecov Report

Merging #21469 (0919173) into main (9862936) will increase coverage by 0.18%.
The diff coverage is 68.20%.

@@            Coverage Diff             @@
##             main   #21469      +/-   ##
==========================================
+ Coverage   47.43%   47.61%   +0.18%     
==========================================
  Files        1020     1023       +3     
  Lines      139036   139354     +318     
==========================================
+ Hits        65945    66359     +414     
+ Misses      65100    64987     -113     
- Partials     7991     8008      +17     
Impacted Files Coverage Δ
models/issues/issue_index.go 40.00% <0.00%> (+6.66%) ⬆️
models/repo.go 51.52% <0.00%> (ø)
modules/base/tool.go 92.75% <ø> (-0.21%) ⬇️
modules/doctor/authorizedkeys.go 12.50% <0.00%> (ø)
routers/api/packages/nuget/api_v3.go 88.70% <ø> (ø)
routers/web/admin/applications.go 0.00% <0.00%> (ø)
routers/web/repo/lfs.go 0.00% <0.00%> (ø)
modules/doctor/heads.go 12.90% <12.90%> (ø)
modules/repository/repo.go 44.73% <33.33%> (ø)
modules/queue/unique_queue_channel.go 40.41% <40.00%> (-0.13%) ⬇️
... and 73 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lunny
Copy link
Member

lunny commented Oct 16, 2022

make L-G-T-M work

@lunny lunny merged commit 6f48a36 into go-gitea:main Oct 16, 2022
@wxiaoguang wxiaoguang deleted the fix-res-index branch October 16, 2022 13:31
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 18, 2022
* upstream/main: (32 commits)
  inline gitpod image (go-gitea#21494)
  [skip ci] Updated translations via Crowdin
  Do not send notifications for draft releases (go-gitea#21451)
  Update reverse-proxies.zh-cn.md (go-gitea#21484)
  Docs: Update the feature comparison to other Git Hosting Services (go-gitea#20933)
  Add some api integration tests (go-gitea#18872)
  probe if sha before exec git (go-gitea#21467)
  Fix incorrect notification commit url (go-gitea#21479)
  Localize all timestamps (go-gitea#21440)
  [skip ci] Updated translations via Crowdin
  Add system setting table with cache and also add cache supports for user setting (go-gitea#18058)
  Return 404 when user is not found on avatar (go-gitea#21476)
  Enforce grouped NuGet search results (go-gitea#21442)
  Display total commit count in hook message (go-gitea#21400)
  Refactor GetNextResourceIndex to make it work properly with transaction (go-gitea#21469)
  Simplify fmt-check (go-gitea#21458)
  update current stable version
  1.17.3 changelog
  [skip ci] Updated translations via Crowdin
  Fix mermaid-related bugs (go-gitea#21431)
  ...
lunny added a commit that referenced this pull request Nov 30, 2022
This PR is a follow up of #21469

Co-authored-by: Lauris BH <lauris@nix.lv>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants