Repository navigation
feat(taskqueue): route PushQueue to Cloud Tasks v2 (GA) - #118
riddhi-shivhare wants to merge 2 commits into
Conversation
21029b3 to
edbe841
Compare
| * Upper bound on waiting for a Cloud Tasks batch operation. Cloud Tasks | ||
| * normally returns batch operations already done. | ||
| */ | ||
| const CLOUD_TASKS_OPERATION_TIMEOUT_MILLIS = 30000; |
There was a problem hiding this comment.
Why we need this? You already know that the batch operations are sync only.
| $host = substr($host, 0, (int) strrpos($host, '.')); | ||
| } | ||
| $parts = explode('.', str_replace('-dot-', '.', $host)); | ||
| array_pop($parts); // The app ID. |
There was a problem hiding this comment.
Verify that the extracted application identifier matches self::getProjectId(); if it does not match, break out of the loop and return [getenv('GAE_SERVICE') ?: null, getenv('GAE_VERSION') ?: null, null].
Domain-scoped project IDs (example.com:my-app -> my-app.example.com.appspot.com) also leave example in $parts, misrouting the task.
There was a problem hiding this comment.
Updated buildCloudTaskRouting to verify that the hostname suffix matches self::getProjectId(), falling back to the current GAE_SERVICE and GAE_VERSION when it does not match. Also mapped domain-scoped project IDs (example.com:my-app to my-app.example.com) before stripping the app suffix and added unit test coverage.
| $region = self::getRegion(); | ||
| $fullQueueName = "projects/" . $projectId . "/locations/" . $region . "/queues/" . $this->name; | ||
|
|
||
| if (count($tasks) === 1) { |
There was a problem hiding this comment.
addTasksCloudTasks() assumes $tasks is a zero-indexed sequential array (0 .. n-1). In PHP, callers frequently pass filtered arrays (such as array_filter(...), which preserves non-zero keys like [1 => $task]) or string-keyed arrays, both of which pass is_array($tasks). When count($tasks) === 1 with a non-zero key, $tasks[0] evaluates to null and causes a fatal error in createSingleTaskCloudTasks(). When count($tasks) > 1, $createTaskRequests is zero-indexed while processBatchCreateResponse() looks up $failedRequests[$idx] using the original keys from $tasks, silently ignoring per-task failures. Normalise $tasks = array_values($tasks); at the entry to addTasks().
There was a problem hiding this comment.
Normalized $tasks with array_values($tasks) at the start of addTasks() and added unit test coverage for non-zero-indexed arrays in both single and batch paths.
|
|
||
| $ownsClient = ($client === null); | ||
| if ($ownsClient) { | ||
| $client = self::newCloudTasksClient($clientClass); |
There was a problem hiding this comment.
Every single-task PushTask::add() call instantiates a new CloudTasksClient and closes its transport in finally. In google/gax, constructing and closing a client per call re-initialises channel credentials, middleware stacks, and transport connections on every enqueue, adding substantial latency when enqueuing multiple individual tasks within a single request. Cache and reuse a lazy static client instance per request (resetting the cached instance when setCloudTasksClientFactory() is called).
There was a problem hiding this comment.
Updated getCloudTasksClient() to lazily cache and reuse a static CloudTasksClient instance across calls within a request, and updated setCloudTasksClientFactory() to close and reset the cached client.
| } | ||
| // The fallback is not cached, so a failed metadata server request (for | ||
| // example a timeout during a cold start) is retried on the next call. | ||
| return getenv('LOCAL_GCP_REGION') ?: 'us-central1'; |
There was a problem hiding this comment.
can you check whether it is us-central1 or just us-central. Test this first, as I remember GAE has us-central only.
There was a problem hiding this comment.
Tested live against Cloud Tasks v2 on rshivhare-demo1 — locations/us-central1 succeeds while locations/us-central fails with 400 INVALID_ARGUMENT: Location 'us-central' is not a valid location. Kept 'us-central1' and added normalizeRegion to map legacy GAE location strings (us-central -> us-central1, europe-west -> europe-west1).
0a0ede8 to
8011fde
Compare
When APPENGINE_USE_CLOUDTASK_PUSH_QUEUE is enabled, PushTask::add() and PushQueue::addTasks() are served by the Cloud Tasks v2 CloudTasksClient instead of the legacy TaskQueue API. The flag is enabled by "true" (case-insensitive) or "1". - A single task uses createTask. - Multiple tasks use batchCreateTasks. Created task names are read from the operation result, and failedRequests in the operation metadata are mapped back to TaskQueue exceptions. - Sub-second delays are preserved in the schedule time. - ALREADY_EXISTS maps to TaskAlreadyExistsException. NOT_FOUND is not treated as an existing task. DEADLINE_EXCEEDED, RESOURCE_EXHAUSTED, ABORTED and UNAVAILABLE map to TransientTaskQueueException. - A Host header that is a hostname of the app sets the task's App Engine routing ([[instance.]version.]service). Without one, a task runs on the current service and version, like a legacy push task without a target. - Host, X-Google-* and X-AppEngine-* headers are not sent, since Cloud Tasks sets them itself. - Batch statuses given as arrays or with string codes are handled. - If batchCreateTasks is not available, tasks are created one at a time and every task is attempted, like BulkAdd. - A region fallback after a failed metadata server request is not cached. - One client is used for all tasks in a call. Adds google/cloud-tasks (^2.3) to composer.json suggest, pins php-coveralls to ^2.7 for PHP 7.2-8.2 CI, and adds unit tests in tests/Api/TaskQueue/PushQueueTest.php.
8011fde to
0419f3e
Compare
gajjarc
left a comment
gajjarc
left a comment
There was a problem hiding this comment.
LGTM, lets wait for the tiny launch doc approval and have the public doc cl ready. Then we can safely merge.
This branch has not been deployed
2 participants
When APPENGINE_USE_CLOUDTASK_PUSH_QUEUE is enabled, PushTask::add() and PushQueue::addTasks() are served by the Cloud Tasks v2 CloudTasksClient instead of the legacy TaskQueue API:
Adds google/cloud-tasks (^2.3) to composer.json suggest, pins php-coveralls to ^2.7 for PHP 7.2-8.2 CI, and adds unit tests in tests/Api/TaskQueue/PushQueueTest.php.