-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: Toggle Runner Version #488
Conversation
Some tests I ran manually: Run
|
8da0e9b
to
23dd326
Compare
So we have decided to pivot away from a full release to a rolling release of V2. As a result, this PR will change to instead be focused on an approach where V1 and V2 share the same stream handlers, allowing Coordinator to affect the Redis streams set on its own, and turn off any executor, V1 or not. This allows to later introduce a allow list to transition indexers to V2 while only requiring allow list visibility to coordinator. |
New use cases tested manually: I believe that by returning a version of -1, we can make the change in Coordinator V2 simple (I might be wrong about this): I've also added a env variable lock on the start and stop endpoints so we can deploy this to whatever env without explicitly enabling V2 functionality until we are ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Just a few comments :)
throw new Error(`Stream handler ${executorId} has no/invalid indexer config.`); | ||
executors.forEach((handler, executorId) => { | ||
let config = handler.getIndexerConfig(); | ||
if (config === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under what conditions is this undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh because the config is read internally 🤔 So we'll get no information for all V1 indexers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. The config is passed in for V2 executors and is used as opposed to pulling config from Redis. So, aside from my hardcoded obviously incorrect version number, the empty config should be another indicator its a V1 indexer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need the account ID/function name so we know which executor to stop, we can probably extract that from the redis stream key, which in this case would be the executorId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep that's correct. The redis stream is the "executorId" for V1 indexers. In fact, list functions API returns the redis stream key as the executor Id for V1 indexers.
The executorId is guaranteed to be correct since its populated from the loop through the map of stream handlers itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in that case can we extract the account_id
and function_name
from the redis stream and populate that information here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't populated it before since I thought it wasn't needed. What's the information used for given executorId is all that's necessary to drop an indexer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The executorId
is all that is needed to stop the executor, but we need to know which indexer that ID corresponds to in order to stop the right one.
We could to the executorId
/redis stream parsing on the Coordinator side, but I want to avoid making that assumption since it won't be true in V2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. Ok, sorry I understand what you're getting at now. Yeah, let me go ahead and do that parsing.
2e04c8b
to
341e5e6
Compare
Example listing of V1 and V2 indexers together. I manually populated streams with 2 and then added one more using the API.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work :)
Runner will need the ability to start using V1 (Polling Redis to start streams) or V2 (Starting streams upon receiving calls to gRPC server) depending on an environment variable we set before deployment. This way, we can make the migration from V1 to V2 and roll that back without too much difficulty.
In order to do this, the main thread needs to have a separate map of stream handlers owned by the grpc server. The server needs to be spun up only in V2 mode. And Redis polling should only happen in V1 mode. In addition, the worker thread needs to only poll Redis stream storage for indexer config in V1 mode. To do so, only V2 will pass in the config into the worker upon executor init. This provides version control over indexer config as well.
Finally, I addressed some remaining TODOs leftover. Mainly returning more information, tracking status of executors, and returning all. information available when listing executors.
For now, I've defaulted the version to V1, if the env variable is not set.