Orleans: Remove Async suffixes from method names?

[sergeybykov]

In the hindsight, I think it was a mistake to follow the guidelines at the time of having "Async" suffixes for async methods. Seems like unnecessary noise now. Should we use v2.0 as an opportunity to remove them all. Or would it be too much work for our users to adjust their code?

[xiazen]

Why it is unnecessary noise, if it was following the guideline?
Is the method name annoy Orleans users in some case?

[ashkan-saeedi-mazdeh]

I guess for parts of Orleans which everything is async (i.e. external user code in grains and grain interfaces, streams ...) it totally makes sense. There is no reason to mark methods Async when all of them have to be async but for internals, I guess still it's useful to let them remain.

In general if everyone is used to the async stuff however and people just remember OnNextAsync OnActivateAsync instead of OnActivate ... we might not want to restart visual memory of everybody. ash to ash, dust to dust fades to black but memory remains :)

If we were starting from scratch then I would suggest removal of async however since it's the default. I prefer to have Synch on the synchronous stuff to warn users actually. When MS had those guidelines async was the new kid around however now in UWP and hopefully soon everywhere else async is the default accepted practice and synchronous code is becoming the rare thing.

[sergeybykov]

It just feels noisy. We don't suggest to add the Async suffix to every grain interface method. So the resulting code looks a bit weird: from within an async grain method (with no Async suffix) I can make an async call to another grain (no suffix), and then I call WriteStateAsync.

I don't feel a strong passion for this. Just wanted to suggest something to consider for 2.0, where we could make such a change.

[ashkan-saeedi-mazdeh]

I think your suggestion is good @sergeybykov and I wonder what others like @galvesribeiro @ReubenBond and @richorama think. I like them being removed to make the API more uniform name-wise.

[galvesribeiro]

I'm with @sergeybykov on that.

When TPL/Task/async/await were introduced, it was a "big change" and people had to migrate from BeginXXX()/EndXXXX() pattern and the "easy" way to make people understand that difference was to make method names XXXAsync(). Now people are used to understand that if a method return Task or Task<T> that method is async so there is no point on having that Async suffix anymore.

So, I vote to remove that on 2.0.

[rrector]

I find the only time the Async suffix is useful is when there is a similarly named synchronous method (e.g. Read and ReadAsync), so it probably makes sense to remove many of the usages in Orleans when the behavior is async only.

However, while I agree in concept with @sergeybykov, I would urge that for a breaking change like this (which is only targeted at cleaning up semantics) that the changes be phased in over multiple versions.

For example, in 2.0.0 add in versions of the methods without the Async suffix and allow either to be used. In 2.0.1, mark the Async versions as [Obsolete]. In some later version, remove them entirely.

With the number of components in Orleans, it can be moderately time consuming to take an updated version. If that update also involves making a bunch of code changes, it can expand dramatically. If we have a grace period where the changes can be farmed out to a number of people in parallel, over a few week period, it makes the upgrade less stressful and easier to schedule.

Granted, this is not feasible for all changes, but where possible it makes a big difference to teams with large investments in Orleans.

[galvesribeiro]

@rrector as much as I agree with you that it is a big change, keep both methods will require a change on multiple public interfaces and change it back when it get deprecated. IMHO it can at first looks like the easy way to do, but that will introduce 2 breaking changes one after the other. IMHO, 2.0 was announced to have all the major breaking changes and that would include a migration plan/tutorial. I think we should do everything once in 2.0 and don't split/break it twice.

If people have to plan to migrate, just plan to migrate once the 2.0 changelog is out.

[rrector]

@galvesribeiro, the planning is not the difficult part. The difficulty is in distributing the work. If it's a breaking change, then the person doing the upgrade must fix all the issues before the upgrade can be checked in. In a phased change approach, one person can do the upgrade, then we can distribute the work to move to the new approach in our ~30 Orleans based services across the 6 different scrum teams that are developing the services.

While I agree that in many cases the Async suffix is redundant, it's not confusing, misleading or inaccurate. There doesn't seem to be sufficient justification to introduce a breaking change because of this.

[rrector]

@sergeybykov, @galvesribeiro I'm frequently impacted by being the one on our team to deal with the breaking changes in Orleans upgrades, so I'm sensitive to this issue. However, let me move from dogma to specifics.

OnActivateAsync and OnDeactivateAsync are firmly established the common nomenclature of grains. Changing these would probably cause the greatest impact and I'd urge not changing them.

OnNextAsync, OnCompletedAsync, and OnErrorAsync in IAsyncObserver are actually canonical examples of the XXXAsync pattern, as they are extending the synchronous concepts in IObserver. Changing these would make it difficult/impossible to ever be able to have a class that implements both IObserver and IAsyncObserver without getting into the hassle of explicit interface implementation. I'd urge leaving these alone.

We pretty much never directly call or implement IStorage, IMemoryStorageGrain, IStorageProvider, IQueueAdapter, IQueueAdapterReceiver, AzureTableDataManager, or SQSStorage. So at least for us impact would me minimal. No objections in updating those.

[gabikliot]

I absolutely agree with Reed, both on the dogma and on the specifics. It's only when u work in a team of 30 people with 5 services at different geo locations do u understand the difficulty in coordination.

On the specifics, we worked extremely hard to explain why async RX is even needed. Even the original RX inventors didn't get it right away. So removing the async suffix now will lead back to the confusion with the original RX.

[galvesribeiro]

Well, I respect your position guys, but I think regardless of team size, we have proper tooling today and this sort of refactory is pretty easy to be done. Anyway, either way for me is is OK, I just think that there are much places in Orleans codebase that don't enforce this XXXAsync policy,the grain developer itself is not forced to do the same and as I mentioned, things changed and today unless you have a 2 methods where one is sync and other async, I don't see a point on have that. Just noise IMHO.

[gabikliot]

What is hard is not to change the code, but get it tested and checked in/merged into multiple places that use that.

[galvesribeiro]

Well, I'm not going to join into a debate... I just think that if we don't change something because it is hard or because it "is working", we would still be using Windows 3.11 Today. But again, this is MHO, and like I said, I'm happy with either way.

[gabikliot]

No one said not to change, neither Reed nor me. Reed asked to change gradually, with [Obsolete].

[dandago]

Just my two cents - I would continue sticking to convention and use the -Async() suffix. Changing this would confuse developers who are used to that convention.

Furthermore, if you were to change these and leave both method names for a while (for backwards compatibility), that would be even more confusing (it would look like there's a sync and an async method, as many libraries have, but that would not be the case).

[sergeybykov]

Great feedback. Thank you all. I'll try to summarize my interpretation.

1. IAsyncObserver methods are nearly impossible to rename because of IObserver. In the hindsight, it was probably a mistake to try to mimic Rx here. It seemed like a good idea at the time. It's too late now. We can completely revisit the API in Streaming v2.
2. Changing IStorageProvider interface would be very disruptive because all providers that are not part of the core codebase will get broken and unusable until updated.
3. OnActivateAsync and OnDeactivateAsync are easiest to change because it would be a simple search-and-replace. However, just renaming them makes little sense.

My tentative conclusion is that we a) have to stay with the existing APIs as is; b) not introduce any new methods with Async suffix; c) not use Async suffix in Streaming v2 and Storage v2 APIs when we get to working on them.