-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 redis sync #4540
Feat redis sync #4540
Conversation
shimonewman
commented
Oct 20, 2022
•
edited
edited
- Cache re-invoking issue
…-sync � Conflicts: � composer.lock
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.
Left my review, please read all comments before starting to adapt the PR as they kind of complete each other.
…to feat-redis-sync � Conflicts: � app/init.php � app/preload.php � composer.json � composer.lock
@@ -0,0 +1,28 @@ | |||
<?php | |||
|
|||
return [ |
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.
This should be updated before merging. No need for default region.
'syncs' => [ | ||
'key' => 'edge', | ||
'name' => 'edge', | ||
'subtitle' => 'Appwrite\'s cache edge sync Endpoint', |
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.
'subtitle' => 'Appwrite\'s cache edge sync Endpoint', | |
'subtitle' => 'TBD, |
'key' => 'edge', | ||
'name' => 'edge', | ||
'subtitle' => 'Appwrite\'s cache edge sync Endpoint', | ||
'description' => 'Cache edge sync Endpoint', |
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.
'description' => 'Cache edge sync Endpoint', | |
'description' => 'TBD', |
->action(function (Request $request) { | ||
|
||
$token = $request->getHeader('authorization'); | ||
$token = str_replace(["Bearer"," "], "", $token); |
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.
Aren't we missing here a space that needs to be removed?
Authorization: Bearer {token}
app/controllers/api/edge.php
Outdated
$fallbackForRedis = AppwriteURL::unparse([ | ||
'scheme' => 'redis', | ||
'host' => App::getEnv('_APP_REDIS_HOST', 'redis'), | ||
'port' => App::getEnv('_APP_REDIS_PORT', '6379'), | ||
'user' => App::getEnv('_APP_REDIS_USER', ''), | ||
'pass' => App::getEnv('_APP_REDIS_PASS', ''), | ||
]); | ||
|
||
$connection = App::getEnv('_APP_CONNECTIONS_QUEUE', $fallbackForRedis); | ||
$dsns = explode(',', $connection ?? ''); | ||
|
||
if (empty($dsns)) { | ||
Console::error("No Dsn found"); | ||
} | ||
|
||
$dsn = explode('=', $dsns[0]); | ||
$dsn = $dsn[1] ?? ''; | ||
$dsn = new DSN($dsn); | ||
|
||
$client = new SyncIn('syncIn', new QueueRedis($dsn->getHost(), $dsn->getPort())); |
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.
This is not how we should implement it. By doing this, you create a new Redis connection on every API call. This should be handled as a resource that can use dependency injection.
app/workers/sync-In.php
Outdated
} | ||
|
||
global $register; | ||
global $dsn; |
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.
Use dependency injection, even if need to copy-paste. We are working on a mechanism for share dependencies between http, realtime, workers, cli.
* @throws Structure | ||
* @throws Exception | ||
*/ | ||
function handle($dbForConsole, $regions, $stack): void |
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.
Very generic name. Let's try to find a better one.
…t-redis-sync � Conflicts: � .env � Dockerfile � app/cli.php � app/init.php � app/preload.php � app/worker.php � composer.json � composer.lock � src/Appwrite/Platform/Tasks/Maintenance.php � src/Appwrite/Platform/Tasks/sync-edge.php
…t-redis-sync � Conflicts: � app/config/regions.php � app/init.php � composer.lock � docker-compose.yml
…t-redis-sync � Conflicts: � composer.lock � docker-compose.yml