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

Revert "Proposing Lean OpenWhisk (#3886)" #4161

Merged
merged 1 commit into from
Dec 7, 2018
Merged

Conversation

rabbah
Copy link
Member

@rabbah rabbah commented Dec 7, 2018

This reverts commit d4a190c.
The reason I'm reverting the commit is because while I can do a local path to mirror some of the changes in the main (fat?) loadbalancer, there is already a fair bit of divergence in the two load balancers because the lean version clones and owns the code and so it's prone to such slippage.

There is a cleaner organization which allows a common LB class which implements much of the logic that is shared between the two implementations. I started to do this on this branch https://github.com/apache/incubator-openwhisk/compare/master...rabbah:patch?expand=1 but it's clear it will take me a bit more time and we shouldn't break all the repos in the meantime.

I further suggest in the refactoring that the lean LB actually implements identity actors for the invoker pool health and feeds so that there's less reason to diverge or special case some of the code. It's best to have one version of this code because it's functionally critical.

@kpavel Sorry that I didn't get to review the code again more recently before it was merged, but I'm happy to help re-introduce this contribution with an improved organization as suggested here if you're agreeable.

Copy link
Member

@dgrove-oss dgrove-oss left a comment

Choose a reason for hiding this comment

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

Agreed. Need to revert, rebase, and refactor.

@rabbah
Copy link
Member Author

rabbah commented Dec 7, 2018

For completeness: We can replay the changes in these two PRs #4135 and #4145 for the lean LB but I prefer not to do that, favoring the more principled approach of reducing duplication.

@codecov-io
Copy link

codecov-io commented Dec 7, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@d4a190c). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4161   +/-   ##
=========================================
  Coverage          ?   81.12%           
=========================================
  Files             ?      152           
  Lines             ?     7291           
  Branches          ?      461           
=========================================
  Hits              ?     5915           
  Misses            ?     1376           
  Partials          ?        0

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 d4a190c...bf0435d. Read the comment docs.

@dgrove-oss dgrove-oss merged commit d1ca290 into apache:master Dec 7, 2018
@kpavel kpavel mentioned this pull request Jan 14, 2019
21 tasks
@rabbah rabbah deleted the lean branch February 8, 2019 02:27
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants