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

remove olympus storage implementation and proxy worker #513

Merged
merged 4 commits into from
Aug 30, 2018

Conversation

rohancme
Copy link
Contributor

Addresses #414

The only worker in the proxy was set up to asynchronously pull from olympus on cache misses. I've deleted all that code in addition to the olympus storage module. As mentioned in #414 if we want to bring back the async download functionality, we can implement a new download.Protocol for olympus

@codecov-io
Copy link

codecov-io commented Aug 18, 2018

Codecov Report

Merging #513 into master will increase coverage by 0.33%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   40.23%   40.56%   +0.33%     
==========================================
  Files         101      100       -1     
  Lines        3127     3057      -70     
==========================================
- Hits         1258     1240      -18     
+ Misses       1742     1693      -49     
+ Partials      127      124       -3
Impacted Files Coverage Δ
cmd/proxy/actions/app.go 52.27% <ø> (-4.49%) ⬇️

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 7af04ff...5bfd95b. Read the comment docs.

@rohancme rohancme mentioned this pull request Aug 19, 2018
@michalpristas
Copy link
Member

please resolve conflicts @rchakra3

Copy link
Contributor

@marwan-at-work marwan-at-work left a comment

Choose a reason for hiding this comment

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

LGTM, just the one comment.

"github.com/gomods/athens/pkg/errors"
"github.com/gomods/athens/pkg/module"
"github.com/gomods/athens/pkg/paths"
)

const (
// OlympusGlobalEndpoint is a default olympus DNS address
OlympusGlobalEndpoint = "http://localhost:3001"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file changed by mistake? this seems out of scope (and already addressed in another PR if im not mistaken)

Copy link
Member

Choose a reason for hiding this comment

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

it migrates const from removed file

},
},
Name: FetcherWorkerName,
MaxConcurrency: env.AthensMaxConcurrency(),
Copy link
Member

Choose a reason for hiding this comment

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

you need to remove this as well from config package

Copy link
Member

Choose a reason for hiding this comment

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

sorry still used in olympus


opts := work.JobOptions{
SkipDead: true,
MaxFails: env.WorkerMaxFails(),
Copy link
Member

Choose a reason for hiding this comment

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

same for WorkerMaxFails

Copy link
Member

Choose a reason for hiding this comment

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

sorry still used in olympus

@rohancme
Copy link
Contributor Author

@marwan-at-work I think your comments aren't applicable after moving the middleware to pkg/

@michalpristas resolved conflicts!

store, err := GetStorage()

Copy link
Member

Choose a reason for hiding this comment

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

can you please remove space?

@michalpristas
Copy link
Member

@marwan-at-work can you please review once more. this is a quick one

@marwan-at-work marwan-at-work merged commit 697a7a5 into gomods:master Aug 30, 2018
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.

4 participants