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

Implement elastic search issue indexer interface #6150

Closed
wants to merge 11 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 21, 2019

No description provided.

@lunny lunny added the type/enhancement An improvement of existing functionality label Feb 21, 2019
@lafriks lafriks added this to the 1.9.0 milestone Feb 21, 2019
@lunny lunny force-pushed the lunny/issue_indexer_es branch 2 times, most recently from bef5278 to 316de4c Compare April 2, 2019 09:37
@lunny lunny changed the title WIP: implement elastic search issue indexer interface Implement elastic search issue indexer interface Apr 2, 2019
@lunny
Copy link
Member Author

lunny commented Apr 2, 2019

It's ready for review now.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 2, 2019
modules/indexer/issues/elastic_search.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 2, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@46d3d10). Click here to learn what that means.
The diff coverage is 7.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6150   +/-   ##
=========================================
  Coverage          ?   40.14%           
=========================================
  Files             ?      404           
  Lines             ?    54263           
  Branches          ?        0           
=========================================
  Hits              ?    21786           
  Misses            ?    29461           
  Partials          ?     3016
Impacted Files Coverage Δ
modules/indexer/issues/elastic_search.go 0% <0%> (ø)
modules/setting/indexer.go 100% <100%> (ø)
modules/indexer/issues/indexer.go 61.9% <50%> (ø)

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 46d3d10...6886071. Read the comment docs.

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.

Hi lunny!

Looks good overall however I think you need to do a general search and replace of elestic for elastic. Done!

Your choice of a skip of 2 for the elastic logger seems unusual. 1 is usually correct as it points to where the Printf is called, unless elastic wraps the Printf itself in which case 2 is appropriate. I've just doublechecked the source. You're right!

You still need to add config stubs to modules/setting/log.go. Basically you need a setting ELASTIC which configures which sublogging modes that would use. You could make this generic and name it INDEXER? Then you could use the same named logger for different types of indexer?

modules/indexer/issues/elastic_search.go Show resolved Hide resolved
@lunny lunny force-pushed the lunny/issue_indexer_es branch from 316de4c to 44637ed Compare April 3, 2019 08:38
@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 Apr 3, 2019

if !exists {
mapping := `{
"settings":{
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if this could be moved out to embedded file so that it could be customized using custom directory

Copy link
Member Author

@lunny lunny Apr 4, 2019

Choose a reason for hiding this comment

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

Maybe a config item on app.ini to point to a mapping file which default are empty is better?

Copy link
Member

Choose a reason for hiding this comment

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

that could also be an option

@lunny lunny force-pushed the lunny/issue_indexer_es branch from 44637ed to 8f20faf Compare April 4, 2019 14:53
.drone.yml Outdated Show resolved Hide resolved
.drone.yml Outdated Show resolved Hide resolved
@Cellebyte
Copy link

@lunny do you know how long this will take to be implemented?
Devs are complaining on the bad codesearch in gitea. I know that DBs and Bleave are not the best search providers.

@lunny
Copy link
Member Author

lunny commented May 8, 2019

These PRs should my next work since migration PR merged. But I can't give you some date.

@Cellebyte
Copy link

Thanks so it will definitly be in the next version 1.9.x?

@lunny lunny force-pushed the lunny/issue_indexer_es branch from ab901c9 to ceec710 Compare May 8, 2019 13:30
@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 May 31, 2019
@lunny lunny force-pushed the lunny/issue_indexer_es branch from ceec710 to d9699ef Compare May 31, 2019 14:40
@techknowlogick techknowlogick added this to the 1.10.0 milestone Jun 4, 2019
@lunny lunny force-pushed the lunny/issue_indexer_es branch from 63e9a4f to cdce7f9 Compare June 10, 2019 11:15
@techknowlogick
Copy link
Member

conflicts

@lunny lunny force-pushed the lunny/issue_indexer_es branch from cdce7f9 to e81618c Compare July 7, 2019 07:39
@lunny lunny force-pushed the lunny/issue_indexer_es branch from e81618c to 6886071 Compare July 9, 2019 06:20
@techknowlogick techknowlogick modified the milestones: 1.10.0, 1.11.0 Sep 3, 2019
@lunny lunny added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Sep 18, 2019
@lunny lunny force-pushed the lunny/issue_indexer_es branch from 8559553 to 40b7f55 Compare November 6, 2019 03:37
@emonty
Copy link
Contributor

emonty commented Nov 6, 2019

@lunny I got this: https://zuul.opendev.org/t/openstack/build/ddba351742a84e66947928231e6a02ff/log/gitea99.opendev.org/docker/giteadocker_gitea-web_1.txt#5319 which maybe looks like the code is trying to create the elasticsearch index twice? there's still something else wrong with that patch, so I'm not 100% sure yet

@emonty
Copy link
Contributor

emonty commented Nov 6, 2019

@lunny nevermind. that's a log line about repo indexer. I think I found another issue in my test job - I'll let you know how the latest version goes

@lunny lunny modified the milestones: 1.11.0, 1.12.0 Dec 13, 2019
@lunny lunny closed this Dec 19, 2019
@lunny lunny deleted the lunny/issue_indexer_es branch December 19, 2019 09:40
@lunny lunny removed this from the 1.12.0 milestone Dec 19, 2019
@lunny
Copy link
Member Author

lunny commented Dec 19, 2019

Replaced by #9428

@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. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants