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.

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

Implement elastic search issue indexer interface #6150

Closed
wants to merge 11 commits into from
Closed

Implement elastic search issue indexer interface #6150

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
apricote
apricote suggested changes Apr 2, 2019
View reviewed changes
modules/indexer/issues/elastic_search.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 2, 2019 •
edited
Loading

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.

zeripath
zeripath reviewed Apr 2, 2019
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment •
edited
Loading

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
@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
lafriks
lafriks reviewed Apr 4, 2019
View reviewed changes
modules/indexer/issues/elastic_search.go Outdated

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 •
edited
Loading

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

apricote
apricote reviewed Apr 4, 2019
View reviewed changes
.drone.yml Outdated Show resolved Hide resolved
apricote
apricote reviewed Apr 5, 2019
View reviewed changes
.drone.yml Outdated Show resolved Hide resolved
@lunny lunny mentioned this pull request Apr 16, 2019
Closed
1 task
@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?

@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
@techknowlogick techknowlogick added this to the 1.10.0 milestone Jun 4, 2019
@techknowlogick
Copy link
Member

conflicts

@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
@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 mentioned this pull request Dec 19, 2019
Merged
@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.
Reviewers

@techknowlogick techknowlogick techknowlogick left review comments

@lafriks lafriks lafriks left review comments

@apricote apricote apricote requested changes

@appleboy appleboy appleboy approved these changes

@zeripath zeripath zeripath approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@lunny @codecov-io @Cellebyte @techknowlogick @emonty @appleboy @lafriks @zeripath @apricote @GiteaBot