Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Make all calls in SchedulerStateManagerAdaptor synchronized #342

Merged
merged 1 commit into from
Apr 13, 2016

Conversation

maosongfu
Copy link
Contributor

  1. Make all calls in SchedulerStateManagerAdaptor synchronized, i.e. wait for the result unless timeout
  2. Correct corresponding invocation of methods in SchedulerStateManagerAdaptor
  3. Fix some misuse of IStateManager or SchedulerStateManagerAdaptor

1. Make all calls in SchedulerStateManagerAdaptor synchronized, i.e. wait for the result unless timeout
2. Correct corresponding invocation of methods in SchedulerStateManagerAdaptor
3. Fix some misuse of IStateManager or SchedulerStateManagerAdaptor
@kramasamy
Copy link
Contributor

👍

@saileshmittal
Copy link
Contributor

This includes changes in the SPI. Won't this break existing scheduler code?

If yes, we probably should keep the existing SPI, and add new interfaces for serial calls. OR we break the SPI this time and communicate to others.

In either case, may we should keep the async version around as well.

@maosongfu
Copy link
Contributor Author

@saileshmittal
Hi,

You are right, changing SPI is possible to break existing scheduler code.
But consider these:

  1. From the start of design, we decided to make all calls in SchedulerStateManagerAdaptor synchronized and there might be some miscommunications or other unknown reasons, it is in async style, which is hard to reason and use in Scheduler implementation case.
  2. I checked current scheduler implementations in the repo and fixed the breaking parts. Async interfaces may not be a good idea in SchedulerStateManagerAdaptor: IStateManager is fully async, and we could expose IStateManager from SchedulerStateManagerAdaptor if in future async methods are really needed.
    Consider we are still in an early stage, it is ok to break things rather than keep async version for backward compatibility concern (which may not even exist at current stage).

@saileshmittal
Copy link
Contributor

ok 👍

@maosongfu maosongfu merged commit dbbb97d into master Apr 13, 2016
@jingwei jingwei deleted the mfu/makeSchedulerStateManagerAdaptorSync branch April 14, 2016 17:08
saileshmittal pushed a commit that referenced this pull request May 18, 2016
1. Make all calls in SchedulerStateManagerAdaptor synchronized, i.e. wait for the result unless timeout
2. Correct corresponding invocation of methods in SchedulerStateManagerAdaptor
3. Fix some misuse of IStateManager or SchedulerStateManagerAdaptor
saileshmittal pushed a commit that referenced this pull request May 19, 2016
1. Make all calls in SchedulerStateManagerAdaptor synchronized, i.e. wait for the result unless timeout
2. Correct corresponding invocation of methods in SchedulerStateManagerAdaptor
3. Fix some misuse of IStateManager or SchedulerStateManagerAdaptor
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants