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

ClientAwaitedAction now uses a channel to notify drops happened #1130

Conversation

allada
Copy link
Member

@allada allada commented Jul 10, 2024

Instead of using Weak references and custom spawns, we now use
a channel to notify receivers that cleanups are needed.


This change is Reviewable

Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

+@adam-singer
+@zbirenbaum

Reviewable status: 0 of 2 LGTMs obtained (waiting on @adam-singer and @zbirenbaum)

@allada allada force-pushed the move-client-awaited-actions-to-use-drop-channels branch 2 times, most recently from f4660df to 08073c9 Compare July 10, 2024 03:56
Copy link
Member

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Reviewed 20 of 20 files at r1, 3 of 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained, and 3 discussions need to be resolved (waiting on @zbirenbaum)


nativelink-scheduler/src/cache_lookup_scheduler.rs line 230 at r1 (raw file):

            match maybe_action_result {
                Ok(action_result) => {
                    println!("{action_result:?}");

nit: remove println


nativelink-scheduler/src/operation_state_manager.rs line 91 at r1 (raw file):

    // /// The operation must have it's last client update before this time.
    // NOTE: NOT PART OF ANY FILTERING!
    // TODO!(cleanup)

Can we leave a note of what should be cleaned up ?


nativelink-scheduler/src/scheduler_state/state_manager.rs line 130 at r3 (raw file):

            .expect("AwaitedAction should be present");
        awaited_action.dec_listening_clients();
        // If we failed to send it means noone is listening.

nit: noone -> none

@allada allada force-pushed the move-client-awaited-actions-to-use-drop-channels branch from 08073c9 to 4359e9b Compare July 10, 2024 17:15
Instead of using Weak references and custom spawns, we now use
a channel to notify receivers that cleanups are needed.
@allada allada force-pushed the move-client-awaited-actions-to-use-drop-channels branch from 4359e9b to d34eab6 Compare July 10, 2024 17:16
Copy link
Member Author

@allada allada left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained, and pending CI: vale, and 2 discussions need to be resolved (waiting on @adam-singer and @zbirenbaum)


nativelink-scheduler/src/cache_lookup_scheduler.rs line 230 at r1 (raw file):

Previously, adam-singer (Adam Singer) wrote…

nit: remove println

Done.

@allada allada merged commit 5e98ec9 into TraceMachina:dev Jul 10, 2024
2 of 3 checks passed
allada added a commit to allada/nativelink-fork that referenced this pull request Jul 16, 2024
This is an extremely significant overhaul to Nativelink's scheduler
component. This new scheduler design is to enable a distributed
scheduling system.

The new components & definitions:
* AwaitedActionDb - An interface that is easier to work with when
  dealing with key-value storage systems.
* MemoryAwaitedActionDb - An in-memory set of hashmaps & btrees used
  to satisfy the requirements of AwaitedActionDb interface.
* ClientStateManager - A minimal interface required to satisfy the
  requirements of a client-facing scheduler.
* WorkerStateManager - A minimal interface required to satisfy the
  requirements of a worker-facing scheduler.
* MatchingEngineStateManager - A minimal interface required to
  satisfy a the engine that matches queued jobs to workers.
* SimpleSchedulerStateManager - An implementation that satisfies
  ClientStateManager, WorkerStateManager & MatchingEngineStateManager
  with all the logic of the previous "SimpleScheduler" logic moved
  behind each interface.
* ApiWorkerScheduler - A component that handles all knowledge about
  workers state and implmenets the WorkerScheduler interface and
  translates them into the WorkerStateManager interface.
* SimpleScheduler - Translation calls of the ClientScheduler
  interface into ClientStateManager & MatchingEngineStateManager.
  This component is currently always fowards calls to
  SimpleSchedulerStateManager then to MemoryAwaitedActionDb.
  Future changes will make these inner components dynamic via config.

In addition we have hardened the interactions of different kind of
IDs in NativeLink. Most relivent is the separation & introduction of:
* OperationId - Represents an individal operation being requested to
  be executed that is unique across all of time.
* ClientOperationId - An ID issued to the client when the client
  requests to execute a job. This ID will point to an OperationId
  internally, but the client is never exposed to the OperationId.
* AwaitedActionHashKey - A key used to uniquely identify an action
  that is not unique across time. This means that this key might
  have multiple OperationId's that have executed it across different
  points in time. This key is used as a "fingerprint" of an operation
  that the client wants to execute and the scheduler may decide to
  join the stream onto an existing operation if this key has a hit.

Overall these changes pave the way for more robust scheduler
implementations, most notably, distributed scheduler implementations
will be easier to impelemnt and will be introduced in followup PRs.

This commit was developed on a side branch and consisted of the
following commits with corresponding code reviews:
54ed73c
    Add scheduler metrics back (TraceMachina#1171)
50fdbd7
    fix formatting (TraceMachina#1170)
8926236
    Merge in main and format (TraceMachina#1168)
9c2c7b9
    key as u64 (TraceMachina#1166)
0192051
    Cleanup unused code and comments (TraceMachina#1165)
080df5d
    Add versioning to AwaitedAction (TraceMachina#1163)
73c19c4
    Fix sequence bug in new memory store manager (TraceMachina#1162)
6e50d2c
    New AwaitedActionDb implementation (TraceMachina#1157)
18db991
    Fix test on running_actions_manager_test (TraceMachina#1141)
e50ef3c
    Rename workers to `worker_scheduler`
1fdd505
    SimpleScheduler now uses config for action pruning (TraceMachina#1137)
eaaa872
    Change encoding for items that are cachable (TraceMachina#1136)
d647056
    Errors are now properly handles in subscription (TraceMachina#1135)
7c3e730
    Restructure files to be more appropriate (TraceMachina#1131)
5e98ec9
    ClientAwaitedAction now uses a channel to notify drops happened (TraceMachina#1130)
52beaf9
    Cleanup unused structs (TraceMachina#1128)
e86fe08
    Remove all uses of salt and put under ActionUniqueQualifier (TraceMachina#1126)
3b86036
    Remove all need for workers to know about ActionId (TraceMachina#1125)
5482d7f
    Fix bazel build and test on dev (TraceMachina#1123)
ba52c7f
    Implement get_action_info to all ActionStateResult impls (TraceMachina#1118)
2fa4fee
    Remove MatchingEngineStateManager::remove_operation (TraceMachina#1119)
34dea06
    Remove unused proto field (TraceMachina#1117)
3070a40
    Remove metrics from new scheduler (TraceMachina#1116)
e95adfc
    StateManager will now cleanup actions on client disconnect (TraceMachina#1107)
6f8c001
    Fix worker execution issues (TraceMachina#1114)
d353c30
    rename set_priority to upgrade_priority (TraceMachina#1112)
0d93671
    StateManager can now be notified of noone listeneing (TraceMachina#1093)
cfc0cf6
    ActionScheduler will now use ActionListener instead of tokio::watch (TraceMachina#1091)
d70d31d
    QA fixes for scheduler-v2 (TraceMachina#1092)
f2cea0c
    [Refactor] Complete rewrite of SimpleScheduler
34d93b7
    [Refactor] Move worker notification in SimpleScheduler under Workers
b9d9702
    [Refactor] Moves worker logic back to SimpleScheduler
7a16e2e
    [Refactor] Move scheduler state behind mute
allada added a commit to allada/nativelink-fork that referenced this pull request Jul 16, 2024
This is an extremely significant overhaul to Nativelink's scheduler
component. This new scheduler design is to enable a distributed
scheduling system.

The new components & definitions:
* AwaitedActionDb - An interface that is easier to work with when
  dealing with key-value storage systems.
* MemoryAwaitedActionDb - An in-memory set of hashmaps & btrees used
  to satisfy the requirements of AwaitedActionDb interface.
* ClientStateManager - A minimal interface required to satisfy the
  requirements of a client-facing scheduler.
* WorkerStateManager - A minimal interface required to satisfy the
  requirements of a worker-facing scheduler.
* MatchingEngineStateManager - A minimal interface required to
  satisfy a engine that matches queued jobs to workers.
* SimpleSchedulerStateManager - An implements that satisfies
  ClientStateManager, WorkerStateManager & MatchingEngineStateManager
  with all the logic of the previous "SimpleScheduler" logic moved
  behind each interface.
* ApiWorkerScheduler - A component that handles all knowledge about
  workers state and implmenets the WorkerScheduler interface and
  translates them into the WorkerStateManager interface.
* SimpleScheduler - Translation calls of the ClientScheduler
  interface into ClientStateManager & MatchingEngineStateManager.
  This component is currently always forwards calls to
  SimpleSchedulerStateManager then to MemoryAwaitedActionDb.
  Future changes will make these inner components dynamic via config.

In addition we have hardened the interactions of different kind of
IDs in NativeLink. Most relevant is the separation & introduction of:
* OperationId - Represents an individual  operation being requested
  to be executed that is unique across all of time.
* ClientOperationId - An ID issued to the client when the client
  requests to execute a job. This ID will point to an OperationId
  internally, but the client is never exposed to the OperationId.
* AwaitedActionHashKey - A key used to uniquely identify an action
  that is not unique across time. This means that this key might
  have multiple OperationId's that have executed it across different
  points in time. This key is used as a "fingerprint" of an operation
  that the client wants to execute and the scheduler may decide to
  join the stream onto an existing operation if this key has a hit.

Overall these changes pave the way for more robust scheduler
implementations, most notably, distributed scheduler implementations
will be easier to implement and will be introduced in followup PRs.

This commit was developed on a side branch and consisted of the
following commits with corresponding code reviews:
54ed73c
    Add scheduler metrics back (TraceMachina#1171)
50fdbd7
    fix formatting (TraceMachina#1170)
8926236
    Merge in main and format (TraceMachina#1168)
9c2c7b9
    key as u64 (TraceMachina#1166)
0192051
    Cleanup unused code and comments (TraceMachina#1165)
080df5d
    Add versioning to AwaitedAction (TraceMachina#1163)
73c19c4
    Fix sequence bug in new memory store manager (TraceMachina#1162)
6e50d2c
    New AwaitedActionDb implementation (TraceMachina#1157)
18db991
    Fix test on running_actions_manager_test (TraceMachina#1141)
e50ef3c
    Rename workers to `worker_scheduler`
1fdd505
    SimpleScheduler now uses config for action pruning (TraceMachina#1137)
eaaa872
    Change encoding for items that are cachable (TraceMachina#1136)
d647056
    Errors are now properly handles in subscription (TraceMachina#1135)
7c3e730
    Restructure files to be more appropriate (TraceMachina#1131)
5e98ec9
    ClientAwaitedAction now uses a channel to notify drops happened (TraceMachina#1130)
52beaf9
    Cleanup unused structs (TraceMachina#1128)
e86fe08
    Remove all uses of salt and put under ActionUniqueQualifier (TraceMachina#1126)
3b86036
    Remove all need for workers to know about ActionId (TraceMachina#1125)
5482d7f
    Fix bazel build and test on dev (TraceMachina#1123)
ba52c7f
    Implement get_action_info to all ActionStateResult impls (TraceMachina#1118)
2fa4fee
    Remove MatchingEngineStateManager::remove_operation (TraceMachina#1119)
34dea06
    Remove unused proto field (TraceMachina#1117)
3070a40
    Remove metrics from new scheduler (TraceMachina#1116)
e95adfc
    StateManager will now cleanup actions on client disconnect (TraceMachina#1107)
6f8c001
    Fix worker execution issues (TraceMachina#1114)
d353c30
    rename set_priority to upgrade_priority (TraceMachina#1112)
0d93671
    StateManager can now be notified of noone listeneing (TraceMachina#1093)
cfc0cf6
    ActionScheduler will now use ActionListener instead of tokio::watch (TraceMachina#1091)
d70d31d
    QA fixes for scheduler-v2 (TraceMachina#1092)
f2cea0c
    [Refactor] Complete rewrite of SimpleScheduler
34d93b7
    [Refactor] Move worker notification in SimpleScheduler under Workers
b9d9702
    [Refactor] Moves worker logic back to SimpleScheduler
7a16e2e
    [Refactor] Move scheduler state behind mute
allada added a commit that referenced this pull request Jul 16, 2024
This is a significant overhaul to Nativelink's scheduler
component. This new scheduler design is to enable a distributed
scheduling system.

The new components & definitions:
* AwaitedActionDb - An interface that is easier to work with when
  dealing with key-value storage systems.
* MemoryAwaitedActionDb - An in-memory set of hashmaps & btrees used
  to satisfy the requirements of AwaitedActionDb interface.
* ClientStateManager - A minimal interface required to satisfy the
  requirements of a client-facing scheduler.
* WorkerStateManager - A minimal interface required to satisfy the
  requirements of a worker-facing scheduler.
* MatchingEngineStateManager - A minimal interface required to
  satisfy a engine that matches queued jobs to workers.
* SimpleSchedulerStateManager - An implements that satisfies
  ClientStateManager, WorkerStateManager & MatchingEngineStateManager
  with all the logic of the previous "SimpleScheduler" logic moved
  behind each interface.
* ApiWorkerScheduler - A component that handles all knowledge about
  workers state and implmenets the WorkerScheduler interface and
  translates them into the WorkerStateManager interface.
* SimpleScheduler - Translation calls of the ClientScheduler
  interface into ClientStateManager & MatchingEngineStateManager.
  This component is currently always forwards calls to
  SimpleSchedulerStateManager then to MemoryAwaitedActionDb.
  Future changes will make these inner components dynamic via config.

In addition we have hardened the interactions of different kind of
IDs in NativeLink. Most relevant is the separation & introduction of:
* OperationId - Represents an individual  operation being requested
  to be executed that is unique across all of time.
* ClientOperationId - An ID issued to the client when the client
  requests to execute a job. This ID will point to an OperationId
  internally, but the client is never exposed to the OperationId.
* AwaitedActionHashKey - A key used to uniquely identify an action
  that is not unique across time. This means that this key might
  have multiple OperationId's that have executed it across different
  points in time. This key is used as a "fingerprint" of an operation
  that the client wants to execute and the scheduler may decide to
  join the stream onto an existing operation if this key has a hit.

Overall these changes pave the way for more robust scheduler
implementations, most notably, distributed scheduler implementations
will be easier to implement and will be introduced in followup PRs.

This commit was developed on a side branch and consisted of the
following commits with corresponding code reviews:
54ed73c
    Add scheduler metrics back (#1171)
50fdbd7
    fix formatting (#1170)
8926236
    Merge in main and format (#1168)
9c2c7b9
    key as u64 (#1166)
0192051
    Cleanup unused code and comments (#1165)
080df5d
    Add versioning to AwaitedAction (#1163)
73c19c4
    Fix sequence bug in new memory store manager (#1162)
6e50d2c
    New AwaitedActionDb implementation (#1157)
18db991
    Fix test on running_actions_manager_test (#1141)
e50ef3c
    Rename workers to `worker_scheduler`
1fdd505
    SimpleScheduler now uses config for action pruning (#1137)
eaaa872
    Change encoding for items that are cachable (#1136)
d647056
    Errors are now properly handles in subscription (#1135)
7c3e730
    Restructure files to be more appropriate (#1131)
5e98ec9
    ClientAwaitedAction now uses a channel to notify drops happened (#1130)
52beaf9
    Cleanup unused structs (#1128)
e86fe08
    Remove all uses of salt and put under ActionUniqueQualifier (#1126)
3b86036
    Remove all need for workers to know about ActionId (#1125)
5482d7f
    Fix bazel build and test on dev (#1123)
ba52c7f
    Implement get_action_info to all ActionStateResult impls (#1118)
2fa4fee
    Remove MatchingEngineStateManager::remove_operation (#1119)
34dea06
    Remove unused proto field (#1117)
3070a40
    Remove metrics from new scheduler (#1116)
e95adfc
    StateManager will now cleanup actions on client disconnect (#1107)
6f8c001
    Fix worker execution issues (#1114)
d353c30
    rename set_priority to upgrade_priority (#1112)
0d93671
    StateManager can now be notified of noone listeneing (#1093)
cfc0cf6
    ActionScheduler will now use ActionListener instead of tokio::watch (#1091)
d70d31d
    QA fixes for scheduler-v2 (#1092)
f2cea0c
    [Refactor] Complete rewrite of SimpleScheduler
34d93b7
    [Refactor] Move worker notification in SimpleScheduler under Workers
b9d9702
    [Refactor] Moves worker logic back to SimpleScheduler
7a16e2e
    [Refactor] Move scheduler state behind mute
allada added a commit that referenced this pull request Jul 17, 2024
This is a significant overhaul to Nativelink's scheduler
component. This new scheduler design is to enable a distributed
scheduling system.

The new components & definitions:
* AwaitedActionDb - An interface that is easier to work with when
  dealing with key-value storage systems.
* MemoryAwaitedActionDb - An in-memory set of hashmaps & btrees used
  to satisfy the requirements of AwaitedActionDb interface.
* ClientStateManager - A minimal interface required to satisfy the
  requirements of a client-facing scheduler.
* WorkerStateManager - A minimal interface required to satisfy the
  requirements of a worker-facing scheduler.
* MatchingEngineStateManager - A minimal interface required to
  satisfy a engine that matches queued jobs to workers.
* SimpleSchedulerStateManager - An implements that satisfies
  ClientStateManager, WorkerStateManager & MatchingEngineStateManager
  with all the logic of the previous "SimpleScheduler" logic moved
  behind each interface.
* ApiWorkerScheduler - A component that handles all knowledge about
  workers state and implmenets the WorkerScheduler interface and
  translates them into the WorkerStateManager interface.
* SimpleScheduler - Translation calls of the ClientScheduler
  interface into ClientStateManager & MatchingEngineStateManager.
  This component is currently always forwards calls to
  SimpleSchedulerStateManager then to MemoryAwaitedActionDb.
  Future changes will make these inner components dynamic via config.

In addition we have hardened the interactions of different kind of
IDs in NativeLink. Most relevant is the separation & introduction of:
* OperationId - Represents an individual  operation being requested
  to be executed that is unique across all of time.
* ClientOperationId - An ID issued to the client when the client
  requests to execute a job. This ID will point to an OperationId
  internally, but the client is never exposed to the OperationId.
* AwaitedActionHashKey - A key used to uniquely identify an action
  that is not unique across time. This means that this key might
  have multiple OperationId's that have executed it across different
  points in time. This key is used as a "fingerprint" of an operation
  that the client wants to execute and the scheduler may decide to
  join the stream onto an existing operation if this key has a hit.

Overall these changes pave the way for more robust scheduler
implementations, most notably, distributed scheduler implementations
will be easier to implement and will be introduced in followup PRs.

This commit was developed on a side branch and consisted of the
following commits with corresponding code reviews:
54ed73c
    Add scheduler metrics back (#1171)
50fdbd7
    fix formatting (#1170)
8926236
    Merge in main and format (#1168)
9c2c7b9
    key as u64 (#1166)
0192051
    Cleanup unused code and comments (#1165)
080df5d
    Add versioning to AwaitedAction (#1163)
73c19c4
    Fix sequence bug in new memory store manager (#1162)
6e50d2c
    New AwaitedActionDb implementation (#1157)
18db991
    Fix test on running_actions_manager_test (#1141)
e50ef3c
    Rename workers to `worker_scheduler`
1fdd505
    SimpleScheduler now uses config for action pruning (#1137)
eaaa872
    Change encoding for items that are cachable (#1136)
d647056
    Errors are now properly handles in subscription (#1135)
7c3e730
    Restructure files to be more appropriate (#1131)
5e98ec9
    ClientAwaitedAction now uses a channel to notify drops happened (#1130)
52beaf9
    Cleanup unused structs (#1128)
e86fe08
    Remove all uses of salt and put under ActionUniqueQualifier (#1126)
3b86036
    Remove all need for workers to know about ActionId (#1125)
5482d7f
    Fix bazel build and test on dev (#1123)
ba52c7f
    Implement get_action_info to all ActionStateResult impls (#1118)
2fa4fee
    Remove MatchingEngineStateManager::remove_operation (#1119)
34dea06
    Remove unused proto field (#1117)
3070a40
    Remove metrics from new scheduler (#1116)
e95adfc
    StateManager will now cleanup actions on client disconnect (#1107)
6f8c001
    Fix worker execution issues (#1114)
d353c30
    rename set_priority to upgrade_priority (#1112)
0d93671
    StateManager can now be notified of noone listeneing (#1093)
cfc0cf6
    ActionScheduler will now use ActionListener instead of tokio::watch (#1091)
d70d31d
    QA fixes for scheduler-v2 (#1092)
f2cea0c
    [Refactor] Complete rewrite of SimpleScheduler
34d93b7
    [Refactor] Move worker notification in SimpleScheduler under Workers
b9d9702
    [Refactor] Moves worker logic back to SimpleScheduler
7a16e2e
    [Refactor] Move scheduler state behind mute
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.

None yet

3 participants