Comments list performance optimization #5305
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f7fa13b to
a1c66ea
Compare
Codecov Report
@@ Coverage Diff @@
## master #5305 +/- ##
==========================================
+ Coverage 41.45% 41.69% +0.23%
==========================================
Files 414 414
Lines 55571 55970 +399
==========================================
+ Hits 23039 23335 +296
- Misses 29434 29506 +72
- Partials 3098 3129 +31
Continue to review full report at Codecov.
|
zeripath
suggested changes
Nov 10, 2018
models/issue_comment.go
Outdated
| return err | ||
| } | ||
|
|
||
| func (c *Comment) loadAttachements(e Engine) (err error) { |
There was a problem hiding this comment.
s/loadAttachements/loadAttachments/g 😉
a1c66ea to
25af6ff
Compare
zeripath
reviewed
Nov 11, 2018
models/issue_comment_list.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // loadAttributes loads all attributes, expect for attachments and comments |
|
How does this work for issues with a large number of comments? It looks like this is going to get everything in one go. |
|
@zeripath This should have better performance than before when the issue has a large number of comments. The PR will reduce many database queries when loading comments attributes. |
9ef3ad9 to
f5856fb
Compare
|
@lunny can you fix remainig issue with 500 error (see drone tests) |
|
I think this enhancement may be included in a following rc of 1.7.0 since this is not a bug/security issue. |
|
@lafriks @JonasFranzDEV will fix this. |
ecac388 to
cdf24fb
Compare
|
@lafriks @jonasfranz fixed. |
cdf24fb to
e2cecbb
Compare
e2cecbb to
66b687e
Compare
|
It's ready to review. |
lafriks
reviewed
Apr 2, 2019
models/issue_comment_list.go
Outdated
| for _, comment := range comments { | ||
| if comment.Issue != nil { | ||
| _, ok := issues[comment.Issue.ID] | ||
| if !ok { |
There was a problem hiding this comment.
nit: This should be like in other functions:
if _, ok := issues[comment.Issue.ID]; !ok {
issues[comment.Issue.ID] = comment.Issue
}
66b687e to
fe5f0b7
Compare
zeripath
approved these changes
Apr 16, 2019
lafriks
approved these changes
Apr 18, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Will also fix #5297