Skip to content

Refactor issue indexer #5363

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Feb 19, 2019
Merged

Refactor issue indexer #5363

merged 17 commits into from
Feb 19, 2019

Conversation

@lunny
Copy link
Member

@lunny lunny commented Nov 20, 2018 •
edited
Loading

This PR refactor issue indexer implementation and create two interface, one is Indexer another is Queue and provide BleveIndexer, ChannelQueuue and LevelQueue. And you can add new implementation after this PR merged.

This PR will keep the issue indexer compitable with lower Gitea version.

EDIT: This PR also fixed some bugs on indexing and added delete index when deleting a repository or future deleting a standalone issue or pull request.

@lunny lunny added type/refactoring Existing code has been cleaned up. There should be no new functionality. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels Nov 20, 2018
@lunny lunny added this to the 1.7.0 milestone Nov 20, 2018
@lunny lunny force-pushed the lunny/indexer_interface branch from 1155231 to c17f19d Compare December 19, 2018 02:01
@lunny lunny changed the title WIP: refactor issue indexer Refactor issue indexer Dec 19, 2018
@codecov-io
Copy link

codecov-io commented Dec 19, 2018 •
edited
Loading

Codecov Report

Merging #5363 into master will decrease coverage by 0.04%.
The diff coverage is 53.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5363      +/-   ##
==========================================
- Coverage   38.84%   38.79%   -0.05%     
==========================================
  Files         349      353       +4     
  Lines       49801    50109     +308     
==========================================
+ Hits        19346    19442      +96     
- Misses      27655    27853     +198     
- Partials     2800     2814      +14
Impacted Files Coverage Δ
models/models.go 51.96% <ø> (-2.59%) ⬇️
models/issue_comment.go 46.33% <ø> (ø) ⬆️
routers/init.go 68.49% <0%> (-3.34%) ⬇️
modules/indexer/issues/queue_channel.go 0% <0%> (ø)
routers/api/v1/repo/issue.go 23.46% <0%> (ø) ⬆️
models/unit_tests.go 70.94% <0%> (-1.87%) ⬇️
modules/setting/indexer.go 100% <100%> (ø)
modules/setting/setting.go 47.77% <100%> (+0.09%) ⬆️
routers/repo/issue.go 36.38% <20%> (-0.13%) ⬇️
modules/notification/indexer/indexer.go 56.6% <50%> (-18.4%) ⬇️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 094263d...0bf2802. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 19, 2018
@techknowlogick techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018
@lunny lunny force-pushed the lunny/indexer_interface branch from d7e7473 to 238bec6 Compare December 20, 2018 13:33
@lunny
Copy link
Member Author

lunny commented Dec 20, 2018

It's ready to review.

@lunny lunny removed the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jan 12, 2019
@lunny lunny force-pushed the lunny/indexer_interface branch 2 times, most recently from 5092ca5 to 5d965a5 Compare January 18, 2019 12:04
@lunny lunny added the type/bug label Jan 19, 2019
@lunny lunny force-pushed the lunny/indexer_interface branch 2 times, most recently from 2d527e9 to 45a6026 Compare February 17, 2019 12:19
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 17, 2019
zeripath
zeripath reviewed Feb 17, 2019
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Just some comments - no hard rejects. I guess my most hard comment is that I'm not sure that the index search functions should really be in models but I'm not stuck.

modules/setting/setting.go Outdated
Copy link
Contributor

@zeripath zeripath Feb 17, 2019

Choose a reason for hiding this comment

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

Can we split this out to modules/setting/indexer.go?

Copy link
Member Author

@lunny lunny Feb 18, 2019

Choose a reason for hiding this comment

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

done.

Gopkg.toml Outdated
Copy link
Contributor

@zeripath zeripath Feb 17, 2019

Choose a reason for hiding this comment

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

This probably should have a new line at the end of this file.

Copy link
Member Author

@lunny lunny Feb 18, 2019

Choose a reason for hiding this comment

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

done.

models/issue_indexer.go Outdated
Copy link
Contributor

@zeripath zeripath Feb 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Probably out of scope for this PR, but I wonder if we really want this functionality in models/ rather than in modules/indexer/. (Obviously out of scope: I think we probably need to restructure models to make it less of a grab-bag of everything and try to move struct into perhaps a core-models and peripheral/plugin system. )

Copy link
Member Author

@lunny lunny Feb 18, 2019

Choose a reason for hiding this comment

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

I would like to do that, but it will change more files. I prefer to do that on another refactor PRs.

@lunny lunny force-pushed the lunny/indexer_interface branch from b4f8e17 to 2e89743 Compare February 18, 2019 03:25
@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 2019
@techknowlogick
Copy link
Member

techknowlogick commented Feb 19, 2019

Moving back to 1.8.0

@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 Feb 19, 2019
@techknowlogick techknowlogick merged commit 830ae61 into go-gitea:master Feb 19, 2019
@lunny lunny deleted the lunny/indexer_interface branch February 19, 2019 14:42
This was referenced Feb 19, 2019
Merged
Closed
Merged
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lafriks lafriks lafriks approved these changes

+1 more reviewer

@zeripath zeripath zeripath approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Milestone

1.8.0

Development

Successfully merging this pull request may close these issues.

7 participants

@lunny @codecov-io @techknowlogick @lafriks @zeripath @bkcsoft @GiteaBot