-
Notifications
You must be signed in to change notification settings - Fork 5
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
Aligning Workers Integration with EFS and DB #263
Conversation
6841441
to
865809c
Compare
As per https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_712994114 and https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_711077414, I'm assuming that the integration of the |
Should centralise all WorkerManager injected tests, all other tests should use no worker manager, explicit Except process tests, but that's a different beast to be tackled in #264. |
@tegefaulkes please update this issue task list as you go along. |
Do we want the |
The The async function createWorkerManager({
cores,
logger,
}: {
cores?: number;
logger?: Logger;
}): Promise<PolykeyWorkerManagerInterface> {
return await WorkerManager.createWorkerManager<PolykeyWorkerModule>({
workerFactory: () => spawn(new Worker('./polykeyWorker')),
cores,
logger,
});
} |
Since the utility function is there, I'm going to move the types in there too from |
@tegefaulkes in the future, beware to use |
The Only 3 places are using worker manager: |
The imports in |
f74ed59
to
e54f351
Compare
@tegefaulkes in the future try to use qualified names:
When importing utilities from a different domain boundary it's better to always use the qualified import because it will allow the code to be understood when posted as snippets. |
Noticed no integration tests of using |
Added `setWorkerManager`, `unsetWorkerManager` methods for `VaultManager` Created `createWorkerManager` utility for creating `PolykeyWorkerManagerInterface`
e54f351
to
7992199
Compare
Some full test failures:
|
The test failure in Running directly |
@joshuakarp I noticed there's a
|
The test failures don't seem to be related to this branch's changes. Using |
That This might be related to https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205#note_695753874. I have some nodes test utils that generate node IDs for specific bucket indexes, and they may be causing random test failures (where the util function isn't creating a node ID for the expected bucket). Will create an issue for it. |
I think that's what it was related to. there are quite a few TODOs and FIXMEs still in the code that should be reviewed and likely removed in most cases. I can make an issue for it and get around to it. |
All good - already made an issue for this specific Re the TODOs and FIXMEs, there's another comment here regarding this in case you haven't seen it @tegefaulkes https://gitlab.com/MatrixAI/Employees/matrix-team/-/issues/8#note_714211497 |
Description
The integration of
js-workers
wasn't entirely clean.The integration style should be following how EFS is integration js-workers.
So there's a bunch of things to remove.
Prior details: https://gitlab.com/MatrixAI/Engineering/Polykey/js-polykey/-/merge_requests/205/#note_712975388
Tasks
src/workers/WorkerManager.test.ts
,src/workers/errors.ts
andtests/workers/WorkerManager.test.ts
src/workers/polykeyWorkerModule.ts
to removeencryptWithKey
anddecryptWithKey
[ ] - ArrayBuffer refactoring for- held off after overall CLI/API reviewsrc/workers/polykeyWorkerModule.ts
workerManager
can be dynamically set forVaultManager
similar toKeyManager
tests/polykeyWorkerModule.test.ts
totests/polykeyWorker.test.ts
and update all test structure to match EFSPolykeyAgent
andKeyManager.test.ts
requires updates to how they construct thePolykeyWorkerManagerInterface
, this can be turned into a utility function if necessary for the default workerFinal checklist