Skip to content

feat: allocating by function executor #1334

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

Merged
merged 10 commits into from
Apr 7, 2025

Conversation

seriousben
Copy link
Member

@seriousben seriousben commented Apr 2, 2025

Context

As part of moving responsibilities from the executors to the server, we need to make the server aware of function executors.

What

This PR replaces the allocations_by_fn indexes by function_executors_by_executor and allocations_by_executor.

Details:

  • Metrics and /internal/allocations are function executor aware
  • ExecutorMetadata only gets upserted if the heartbeat state hash changes.
  • ExecutorUpserted state change now triggers reconcilation in the graph processor
  • Current executor reconciliation does two things: 1) ensure all current FE respect current dev + allow list + 2) remove any FE that has been removed on the executors. We also reallocate immediately any tasks that gets deallocated.
  • Task Allocation now is structured differently:
      1. get_executor_candidates
      1. select_executor
      1. ensure function executor
      1. create allocation
  • A new migration reallocates any existing allocation in order to make sure all allocations have a function executor ID associated to them

Testing

  • Test Executor upsert + reconciliation logic
  • Test allocation leveraging different function executors

Contribution Checklist

  • If a Python package was changed, please run make fmt in the package directory.
  • If the server was changed, please run make fmt in server/.
  • Make sure all PR Checks are passing.

@seriousben seriousben marked this pull request as draft April 2, 2025 21:05
@seriousben seriousben force-pushed the seriousben/allocate-using-function-executor branch from 756627a to f501d85 Compare April 3, 2025 20:53
return Ok(StateMachineMetadata {
db_version: 0,
db_version: SERVER_DB_VERSION,
Copy link
Member Author

@seriousben seriousben Apr 3, 2025

Choose a reason for hiding this comment

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

This was a gap before. We would start any DB on version zero without migrating.

@seriousben seriousben marked this pull request as ready for review April 3, 2025 21:22
@eabatalov
Copy link
Contributor

fyi, test failed:

======================================================================
FAIL: test_function_routing (__main__.TestFunctionAllowlist)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/indexify/indexify/indexify/tests/./cli/test_function_allowlist.py", line 227, in test_function_routing
    assert (
AssertionError: Not all executors were used: {'executor_dev': 800, 'executor_a': 0, 'executor_b': 0, 'executor_c': 0}

Copy link
Contributor

@eabatalov eabatalov left a comment

Choose a reason for hiding this comment

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

Approving but please fix the test failure unless it's a known issue. Seems like it's not.

@seriousben seriousben force-pushed the seriousben/allocate-using-function-executor branch from ecf758d to 2e20674 Compare April 7, 2025 15:53
@seriousben seriousben force-pushed the seriousben/allocate-using-function-executor branch from bda8c05 to 7432133 Compare April 7, 2025 16:00
@seriousben seriousben merged commit 871346f into main Apr 7, 2025
9 checks passed
@seriousben seriousben deleted the seriousben/allocate-using-function-executor branch April 7, 2025 20:30
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.

3 participants