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

Move more requires to autoload #85

Merged
merged 1 commit into from
Apr 10, 2015

Conversation

plribeiro3000
Copy link
Member

On the last PR i left some requires that could been using autoload to improve the performance over the issue with fog/fog#3430.

@geemus
Copy link
Member

geemus commented Apr 10, 2015

Cool, thanks! Do you think this will help with the route53 cookbook issue?

geemus added a commit that referenced this pull request Apr 10, 2015
@geemus geemus merged commit cb4dd3b into fog:master Apr 10, 2015
@plribeiro3000 plribeiro3000 deleted the autoloading-more-stuff branch April 10, 2015 15:08
@plribeiro3000
Copy link
Member Author

@geemus Perhaps. But i fear the issue is related to the missing require for fog/core in lib/fog/aws/storage after my changes. If the latest version of route53 still have this issue we can just put back the require and warning all libs that depend on fog of this ongoing change in fog.

Before the extraction the require did make some sense because fog has a lazy load mechanism and the dependent libs could do a require for only the service they want to reduce memory usage.
Since we started moving to gemnized providers, this is not valid anymore because the user can just list the providers they want instead of fog and just the service they use will be loaded because of autoload.

So i believe this is an issue because of they way fog used to work and the new way it is heading.
Perhaps we might have to plain it a little more and take this kind of stuff in account before moving.

For now i think that we should fix the issue and try to not follow the old style again since it does not fit the new architecture. But perhaps we can do it for a while until we get everything working as expected.

If we don't get a response/feedback related to the route53 version being used i will add the require back as a temporary workaround.

@geemus
Copy link
Member

geemus commented Apr 10, 2015

Got it, thanks for the update!

On Fri, Apr 10, 2015 at 10:15 AM, Paulo Henrique Lopes Ribeiro <
notifications@github.com> wrote:

@geemus https://github.com/geemus Perhaps. But i fear the issue is
related to the missing require for fog/core in lib/fog/aws/storage after
my changes. If the latest version of route53 still have this issue we can
just put back the require and warning all libs that depend on fog of this
ongoing change in fog.

Before the extraction the require did make some sense because fog has a
lazy load mechanism and the dependent libs could do a require for only the
service they want to reduce memory usage.
Since we started moving to gemnized providers, this is not valid anymore
because the user can just list the providers they want instead of fog and
just the service they use will be loaded because of autoload.

So i believe this is an issue because of they way fog used to work and
the new way it is heading.
Perhaps we might have to plain it a little more and take this kind of
stuff in account before moving.

For now i think that we should fix the issue and try to not follow the old
style again since it does not fit the new architecture. But perhaps we can
do it for a while until we get everything working as expected.

If we don't get a response/feedback related to the route53 version being
used i will add the require back as a temporary workaround.

Reply to this email directly or view it on GitHub
#85 (comment).

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.

2 participants