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 issue indexer #5363

Merged
merged 17 commits into from
Feb 19, 2019
Merged

Conversation

lunny
Copy link
Member

@lunny lunny commented Nov 20, 2018

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 changed the title WIP: refactor issue indexer Refactor issue indexer Dec 19, 2018
@codecov-io
Copy link

codecov-io commented Dec 19, 2018

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
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
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.

@@ -1215,6 +1232,22 @@ func NewContext() {
// Explicitly disable credential helper, otherwise Git credentials might leak
git.GlobalCommandArgs = append(git.GlobalCommandArgs, "-c", "credential.helper=")
}

sec = Cfg.Section("indexer")
Copy link
Contributor

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

Choose a reason for hiding this comment

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

done.

Gopkg.toml Outdated
@@ -119,4 +119,4 @@ ignored = ["google.golang.org/appengine*"]

[[constraint]]
name = "github.com/prometheus/client_golang"
version = "0.9.0"
version = "0.9.0"
Copy link
Contributor

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

Choose a reason for hiding this comment

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

done.

@@ -7,25 +7,60 @@ package models
import (
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.

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

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.

@techknowlogick techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 2019
@techknowlogick
Copy link
Member

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
@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.
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
Development

Successfully merging this pull request may close these issues.

7 participants