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

Performance: do not fetch extra post data unless needed #105

Merged
merged 9 commits into from
Apr 25, 2020

Conversation

mo2menelzeiny
Copy link
Contributor

Summary

  1. Lazily resolves the scope channel and team only when the scope is defined
  2. Adds gating conditions rather than nested if
  3. Moves the API call logic to a separate function
  4. Increases coverage on ProcessPosts

Ticket Link

Closes #87

@levb levb requested review from levb and iomodo April 7, 2020 23:31
@levb levb added the 2: Dev Review Requires review by a core committer label Apr 7, 2020
Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! One style suggestion.

server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved
@mo2menelzeiny mo2menelzeiny requested a review from levb April 8, 2020 09:51
Copy link
Contributor

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @mo2menelzeiny.
Let me understand the overall logic here. We traverse the markdown tree with Inspect which basically calls passed function for every node(let's say N nodes). In the function itself for each link where we have a scope we call the inScope method. Let's assume we have M links, this makes O(N*M) calls of the inScope(and resolveScope respectively).
Return values of team.Name and channel.Name will be the same for every call. Am I missing something? Why not call resolveScope method just once before the Inspect?

@mo2menelzeiny
Copy link
Contributor Author

mo2menelzeiny commented Apr 8, 2020

Thanks for the contribution @mo2menelzeiny.
Let me understand the overall logic here. We traverse the markdown tree with Inspect which basically calls passed function for every node(let's say N nodes). In the function itself for each link where we have a scope we call the inScope method. Let's assume we have M links, this makes O(N*M) calls of the inScope(and resolveScope respectively).
Return values of team.Name and channel.Name will be the same for every call. Am I missing something? Why not call resolveScope method just once before the Inspect?

@iomodo I don't know how I missed this, you're definitely right.

refactor contains function and make it more readable
@mo2menelzeiny mo2menelzeiny requested a review from iomodo April 8, 2020 11:54
Copy link
Contributor

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

Awesome @mo2menelzeiny !
Just two minor improvements below.

server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved
server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved
@mo2menelzeiny mo2menelzeiny requested a review from iomodo April 8, 2020 13:16
Copy link
Contributor

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

@mo2menelzeiny, Sorry for being pain in the neck, couple more very minor comments below :)

server/autolinkplugin/plugin.go Show resolved Hide resolved
server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved
server/autolinkplugin/plugin.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 8, 2020

Codecov Report

Merging #105 into master will increase coverage by 5.46%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   42.62%   48.09%   +5.46%     
==========================================
  Files           6        6              
  Lines         563      576      +13     
==========================================
+ Hits          240      277      +37     
+ Misses        303      282      -21     
+ Partials       20       17       -3     
Impacted Files Coverage Δ
server/autolinkplugin/plugin.go 92.00% <100.00%> (+22.35%) ⬆️

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 384f220...806a1d8. Read the comment docs.

@mo2menelzeiny mo2menelzeiny requested a review from iomodo April 8, 2020 15:03
Copy link
Contributor

@iomodo iomodo left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mo2menelzeiny ! Great work!

@levb levb requested a review from DHaussermann April 8, 2020 16:47
@levb levb added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Apr 8, 2020
@mo2menelzeiny
Copy link
Contributor Author

Hey guys, @DHaussermann @levb @iomodo
are there any troubles with this?

@levb
Copy link
Contributor

levb commented Apr 15, 2020

@mo2menelzeiny Apologies for the wait, but we are heavily backlogged in QA, and this requires QA verification before it's merged.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Regression tested that scope is applied correctly to Teams and combinations of Teams and Channels
  • Tested that scope is correctly applied when added and updated
  • Tested that scope rules are correctly applied when updating existing post
  • Other brief regression testing.
    LGTM!

Sorry for the delay on testing this PR @mo2menelzeiny. Huge thanks for this improvement!

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels Apr 23, 2020
@mo2menelzeiny
Copy link
Contributor Author

@levb @iomodo @DHaussermann
I think the pull request is now ready for merging, am I right?

@iomodo iomodo merged commit 6e6b5df into mattermost-community:master Apr 25, 2020
@hanzei hanzei added this to the v1.3.0 milestone Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance: do not fetch extra post data unless needed
6 participants