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

Ambiguity in asynchronous execution setting #27

Open
Temikus opened this issue Apr 26, 2015 · 13 comments
Open

Ambiguity in asynchronous execution setting #27

Temikus opened this issue Apr 26, 2015 · 13 comments

Comments

@Temikus
Copy link
Member

Temikus commented Apr 26, 2015

While figuring out acceptance tests for vagrant-google, I found a weird logic piece in the implementation of synchronous operations in Fog.
As an example, let's take a look at Server class' destroy method:

def destroy(async=true)
  requires :name, :zone

  data = service.delete_server(name, zone_name)
  operation = Fog::Compute::Google::Operations.new(:service => service).get(data.body['name'], data.body['zone'])
  unless async
    operation.wait_for { ready? }
  end
  operation
end

The async parameter is just a true/false switch, so if we need to perform the operation synchronously (important for tests for example), we need to specify it like so:

instance.destroy(false)

Which, I find highly confusing to understand for someone who's reading the code later.
Due to the default being true, it is not easy to wrap around with a statement, since it will not make any sense either:

async_execution = false
instance.destroy(async_execution)

I was wandering - maybe it makes sense to make it a named parameter?
This will allow for:

  1. Logically sound statements:
instance.start(async: false)
  1. Ability to write in more execution flow control parameters easily if we need them.
@ikehz ikehz added this to the v1.0.0 milestone Apr 27, 2015
@ikehz
Copy link
Member

ikehz commented Apr 27, 2015

@Temikus I like this a lot; I've noticed this as well. However, I don't believe your proposed solution is possible until we've dropped support for Ruby 1. Until then, we'd have to take an optional hash argument instead of using a named parameter. Fog has no plans to drop support for Ruby 1.9, (only 1.8,) as far as I know.

@plribeiro3000 is there precedent for this in other fog projects?

@ikehz
Copy link
Member

ikehz commented Apr 27, 2015

Regardless of implementation, this is a breaking change, so I've marked it for v1.0.0.

@plribeiro3000
Copy link
Member

@ihmccreery we are following the main repo conventions. I believe this would give us less issues like a provider that supports a ruby version that the main one does not.

For now we will have to keep all 1.8.7 compatible but in the next big release we will drop it.

@ikehz
Copy link
Member

ikehz commented Apr 27, 2015

@plribeiro3000 Sorry, that wasn't clear. I know we need to keep support for 1.8.7 until fog v2. I meant to ask you about precedent for sync/async parameters for operations in other providers.

@plribeiro3000
Copy link
Member

Oh, gotcha. I'm not sure about conventions on this one but usually @geemus always ask to drop boolean params that change the behavior of the method. I tend to agree and define 2 methods with proper names to indicate the exact behavior of each one.

Does that answer your question? (Sorry for the miss on the first attempt)

@geemus
Copy link
Member

geemus commented Apr 27, 2015

Yeah, tricky. I think as @plribeiro3000 suggests, we could add async_destroy and sync_destroy or something to help make it really clear. What do you guys think? We could just leave the existing behavior on the existing method that way, but still win some improvement. Thoughts?

@Temikus
Copy link
Member Author

Temikus commented Apr 28, 2015

I agree with separating into methods. It seems better than implementing hash option fetching.
However, what about the existing method? Will it eventually produce a deprecation warning? Or async_destroy and sync_destroy will be left as helper methods until 1.9.3 will be deprecated (it's bound to happen at some point in the future, even though it may be quite remote).

@ikehz
Copy link
Member

ikehz commented Apr 28, 2015

Hm. Separate methods are generally a good thing, I think, but it does mean we won't be able to wrap blocks in sync or async rules. Anything like this would no longer be easy:

async_execution = false
instance.destroy(async_execution)

Whatever we decide, I think it makes sense to see a deprecation warning in v0.1.0, and full removal of the current behavior in v1.0.0, which hopefully would sync with fog v2.

@Temikus
Copy link
Member Author

Temikus commented Apr 28, 2015

But that's what I was saying. async_execution = false reads like "this is not an asynchronous operation". With the async meaning "not synchronous" logically, it starts to read really confusing when we go to the method and I cannot see a good wrapper around it. Since it's false, it will always be the opposite of what the block is labeled, as with your example:

async_execution = false
instance.destroy(async_execution)

It reads "set the async_execution to false", "destroy instance with async execution". Which is super confusing.

If we had the sync which amounted to true, however, it would still be a bit less readable, but would actually make sense logically, as in:

sync_execution = true
instance.destroy(sync_execution)

or

override_default_behavior = true
instance.destroy(override_default_behavior)

@ikehz
Copy link
Member

ikehz commented Apr 28, 2015

Yeah, I see what you're saying. Would just a simple variable rename, like

sync_execution_switch = false
instance.destroy(sync_execution_switch)

or, in params world

sync_execution_switch = false
instance.destroy(:async => sync_execution_switch)

solve the issue? I'm could also imagine something more involved, like

instance.sync { instance.destroy }

versus

instance.async { instance.destroy }

but that's a pretty big syntax change.

To be clear, I think this should change, I'm just skeptical about the multiple method names. That said, I'm happy to be overruled.

@icco icco removed this from the v1.0.0 milestone Jan 9, 2018
@Temikus Temikus added this to the 2.0 milestone Jun 23, 2018
@Temikus
Copy link
Member Author

Temikus commented Jun 23, 2018

I think we can just reformat this as a positional parameter across the board.

Game plan for 2.0:

  • Add async: named parameter to all .create and .destroy methods, i.e.: instance.destroy(async: false)
  • Keep backwards compatibility during 2.0 but add deprecation warnings.
  • Switch integration tests to use named parameters.
  • Add unit tests to enforce behavior in new added models.

For 3.0:

  • Remove all compatibility methods and switch to new behaviour.

The only thing I'm concerned about is if there are any weird corner cases with mixing positional and named parameters...

@icco WDYT?

@icco
Copy link
Member

icco commented Jun 27, 2018

Seems like a solid plan @Temikus

@github-actions
Copy link

This issue has been marked inactive and will be closed if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants