Add simple demo framework for Bedrock::Jobs; add requestID; add retries #15

quinthar · 2017-04-12T20:59:47Z

This does the following:

Adds a simple script that demonstrates Bedrock::Jobs with a trivial worker.
Includes a few minor internal cleanups, with comments on each inside the diff.
Provides a new API to fix https://github.com/Expensify/Expensify/issues/50199 (see https://github.com/Expensify/Web-Expensify/pull/17025)
Fixes https://github.com/Expensify/Expensify/issues/50137

Sorry for globbing multiple issues and cleanups into a single PR; I wanted to use the demo harness I built to solve those other issues.

Assigning to @iwiznia for review this and https://github.com/Expensify/Web-Expensify/pull/17025

quinthar · 2017-04-12T21:00:19Z

bin/BedrockWorkerManager.php

@@ -71,34 +71,37 @@
 Client::configure($bedrockConfig);

 $logger = Client::getLogger();
+$logger->info('Starting BedrockWorkerManager', ['maxLoopIteration' => $maxLoopIteration]);


Just moving this line further up to keep the logging initialization code as early as possible.

quinthar · 2017-04-12T21:00:51Z

bin/BedrockWorkerManager.php

        if ($loopIteration === $maxLoopIteration) {
            $logger->info("We did all our loops iteration, shutting down");
-            exit(0);
+            break;


exit(0) skips the "wait for children to finish" code at the end.

quinthar · 2017-04-12T21:45:33Z

bin/BedrockWorkerManager.php

        $loopIteration++;
        $logger->info("Loop iteration", ['iteration' => $loopIteration]);

        // Step One wait for resources to free up
        while (true) {
            // Get the latest load
+            if (!file_exists('/proc/loadavg')) {
+                throw new Exception('are you in a chroot?  If so, please make sure /proc is mounted correctly');
+            }


Moving the /proc/loadavg check here to keep it next to the sys_getloadavg() call that depends on it.

I don't think we need to do this check every time the loop runs.

Agreed we don't need to, but I think it's non-harmful to do it (eg, it's not a super expensive call given the relatively low the frequency of this loop), and makes the code read a lot cleaner to keep it where it's used.

quinthar · 2017-04-12T21:46:18Z

bin/BedrockWorkerManager.php

            clearstatcache(true, $versionWatchFile);
-            $newVersionWatchFileTimestamp = $versionWatchFile && file_exists($versionWatchFile) ? filemtime($versionWatchFile) : false;
+            $newVersionWatchFileTimestamp = ($versionWatchFile && file_exists($versionWatchFile)) ? filemtime($versionWatchFile) : false;


Just clarifying what this means with parenthesis as the precedence of the operators wasn't obvious to me at first.

quinthar · 2017-04-12T21:46:50Z

bin/BedrockWorkerManager.php

+$logger->info('Stopping BedrockWorkerManager, waiting for children');
+$status = null;
+pcntl_wait($status);
+$logger->info('Stopped BedrockWorkerManager');


Moving all this logic to the end of the outer loop, so it runs regardless of whether we're doing a graceful or abnormal shutdown.

@iwiznia Are you sure this shutdown code does anything?

quinthar · 2017-04-12T21:48:38Z

bin/BedrockWorkerManager.php

@@ -71,34 +71,37 @@
 Client::configure($bedrockConfig);


@iwiznia - Why do you manually copy all the values over to $bedrockConfig before passing it in, rather than just passing in $options?

quinthar · 2017-04-13T01:00:33Z

bin/BedrockWorkerManager.php

 }
+
+// Add defaults
+$jobName = @$options['jobName'] ?: '*'; // Process all jobs by default


Moved this up closer to the usage description and updated the defaults to work out of the box with a minimum number of command line parameters.

quinthar · 2017-04-13T01:01:19Z

bin/BedrockWorkerManager.php

    while (true) {
-        if ($loopIteration === $maxLoopIteration) {
+        // Is it time to self destruct?
+        if ($maxIterations > 0 && $iteration >= $maxIterations) {


Renamed these variables to just use iterations rather than loopIterations.

quinthar · 2017-04-13T01:03:05Z

sample/SampleWorker.php

+/**
+ * Sample BedrockWorkerManager worker class.
+ */
+class SampleWorker extends BedrockWorker


Adding this to provide a quick test and demonstration harness for how BWM works. In doing so I discovered that the Bedrock-PHP repo is incomplete, as BedrockWorker isn't included here. We need to extract that from the Expensify codebase and include in Bedrock-PHP/src instead.

If you are not moving BedrockWorker to this repo nor removing the Log call, I say we just leave the SampleWorker in the web repo for now and move it whenever we do that.

How about I just move SampleWorker to Bedrock-PHP as part of this PR, and remove its call to Log::, but we leave BedrockWorker where it is for now. So I'll make partial progress.

quinthar · 2017-04-13T01:03:21Z

sample/SampleWorker.php

+     */
+    public function run()
+    {
+        Log::info("Running SampleWorker for '{$this->job['name']}'");


This is a call to the Expensify Log class -- need to remove at some point, but I'm not doing it here.

quinthar · 2017-04-13T08:04:15Z

bin/BedrockWorkerManager.php

+$maxIterations = intval(@$options['maxIterations']) ?: -1; // Unlimited iterations by default
+
+// Configure the Bedrock client with these command-line options
+Client::configure($options);


I didn't see the value of $bedrockConfig as it seems to just be a direct copy of $options so I removed it. Please let me know if there was some hidden intent behind it, otherwise I'm assuming it was cruft (eg, maybe it previously had some kind of value, but some change obviated that value and this was just left over as an obsolete relic of the past).

quinthar · 2017-04-13T08:06:08Z

src/Client.php

+    public static function setGlobalRequestID($globalRequestID)
+    {
+        // Override the global requestID
+        self::$globalRequestID = $globalRequestID;


Note: this is called in Web-Expensify/_config.php and Web-Expensify/lib/Logger.php

quinthar · 2017-04-13T08:07:05Z

src/Jobs.php

+                "jobID"      => $jobID,
+                "data"       => $data,
+                "repeat"     => $repeat,
+                "idempotent" => "true",


This header just indicates that it's safe to execute this command twice, as the result will be the same.

quinthar · 2017-04-13T08:13:01Z

bin/BedrockWorkerManager.php

+// Add defaults
+$jobName = @$options['jobName'] ?: '*'; // Process all jobs by default
+$maxLoad = floatval(@$options['maxLoad']) ?: 1.0; // Max load of 1.0 by default
+$maxIterations = intval(@$options['maxIterations']) ?: -1; // Unlimited iterations by default


I had a brief conversation with @cead22 about the "error control operator". I understand that we discourage it (though I don't understand why). Regardless, so far as I can tell we don't explicitly forbid it -- just discourage it -- which I'm interpreting as meaning it's up to the programmer's judgement. And in my judgement, the code above is far more readable than the alternative:

$jobName = isset($options['jobName']) ? $options['jobName'] : '*'; $maxLoad = (isset($options['maxLoad'] && floatval($options['maxLoad'])) ? floatval($options['maxLoad']) : 1.0; $maxIterations = (isset($options['maxIterations'] && intval($options['maxIterations'])) ? intval($options['maxIterations']) ? -1;

Now that we are on PHP7, we use the null coalescing operator and write this as $options['user'] ?? '*', which is better as it doesn't produce a silent notice.

iwiznia · 2017-04-13T09:49:11Z

bin/BedrockWorkerManager.php

+// Add defaults
+$jobName = @$options['jobName'] ?: '*'; // Process all jobs by default
+$maxLoad = floatval(@$options['maxLoad']) ?: 1.0; // Max load of 1.0 by default
+$maxIterations = intval(@$options['maxIterations']) ?: -1; // Unlimited iterations by default


Now that we are on PHP7, we use the null coalescing operator and write this as $options['user'] ?? '*', which is better as it doesn't produce a silent notice.

iwiznia · 2017-04-13T09:53:07Z

bin/BedrockWorkerManager.php


+// Configure the Bedrock client with these command-line options
+Client::configure($options);


NAB: we were copying the bedrock specific configurations to a new array in order to not pass a ton of garbage options to the bedrock client. Now all bwm specific params are being passed to the client. Anyway, since I can't see any problem with it, I think is fine as is.

iwiznia · 2017-04-13T10:00:40Z

bin/BedrockWorkerManager.php

        $loopIteration++;
        $logger->info("Loop iteration", ['iteration' => $loopIteration]);

        // Step One wait for resources to free up
        while (true) {
            // Get the latest load
+            if (!file_exists('/proc/loadavg')) {
+                throw new Exception('are you in a chroot?  If so, please make sure /proc is mounted correctly');
+            }


I don't think we need to do this check every time the loop runs.

iwiznia · 2017-04-13T10:03:03Z

bin/BedrockWorkerManager.php

+$logger->info('Stopping BedrockWorkerManager, waiting for children');
+$status = null;
+pcntl_wait($status);
+$logger->info('Stopped BedrockWorkerManager');


iwiznia · 2017-04-13T10:04:41Z

sample/SampleWorker.php

+/**
+ * Sample BedrockWorkerManager worker class.
+ */
+class SampleWorker extends BedrockWorker


If you are not moving BedrockWorker to this repo nor removing the Log call, I say we just leave the SampleWorker in the web repo for now and move it whenever we do that.

iwiznia · 2017-04-13T10:12:52Z

src/Client.php

+        // Try idempotent requests up to three times, everything else only once
+        $numTries = (@$headers['idempotent'] === 'true') ? 3 : 1;
+        $response = null;
+        while($numTries-- && !$response)


Style: all these { should be on the same line.

iwiznia · 2017-04-13T10:13:19Z

src/Client.php

+                if ($numTries) {
+                    $this->getLogger()->warning("Failed to connect, send, or receive request; retrying $numTries more times", ['message' => $e->getMessage()]);
+                } else {
+                    $this->getLogger()->warning("Failed to connect, send, or receive request; not retrying", ['message' => $e->getMessage()]);


Log error instead of warning?

Hm, well warning implies "this is something bad but not fatal", error implies "this is fatal and non-recoverable" to me.

That's my point, when you retry, it's a warning, when you stopped retrying, is an error.

Ahh got it, will do.

iwiznia · 2017-04-13T10:18:27Z

src/Client.php

@@ -146,6 +156,8 @@ public function call($method, $headers = [], $body = '')
        // Include the requestID for logging purposes
        if ($this->requestID) {
            $headers['requestID'] = $this->requestID;
+        } else if (self::$globalRequestID) {


Does this work? I think $this->requestID will always be set, won't it?

How REQUEST_ID works today:

Actually no, it's not set by default. It's set to null in configure() by default, and that default is not overridden in BedrockWorkerManager.php. Indeed, I don't actually think that $this->requestID is set anywhere.

The closest we seem to do is this totally insane hack, but that only sets the requestID for Log::info(): it doesn't tell Bedrock to send it.

And the only reason Auth gets it is because we don't use Bedrock\Client and don't look at Log. Rather, we just use set it equal to the global REQUEST_ID constant directly.

What this PR does:

So, I've updated _config.php to set requestID so it's automatically set at the very start of the script, and also setting it inside the crazy hack.

Unfortunately, I'm realizing now what sucks about this is we will not update the REQUEST_ID constant -- it'll still be set to the REQUEST_ID of the parent BWM process. So this PR will create traceability through Bedrock, but not into Auth.

Proposing a better solution:

So I think the real solution would be to:

Replace the REQUEST_ID constant with a $REQUEST_ID global variable, and then just use that everywhere. (I'm not sure why we're setting it separately inside Log -- we should just use the global.) This would also allow us to remove the crazy hack, which would be great. This would be pretty straightforward -- it's not actually used in that many places:

Davids-MacBook-Pro:Web-Expensify dbarrett$ grep -s -r REQUEST_ID * _config.php: * REQUEST_ID yet. This will be set with the logID request parameter if it is _config.php:if (!defined('REQUEST_ID') && _config.php: define('REQUEST_ID', $_REQUEST['logID']); _config.php: define('REQUEST_ID', Log::generateID()); _config.php:Log::init(REQUEST_ID); _config.php:Client::setGlobalRequestID(REQUEST_ID); _config.php: newrelic_add_custom_parameter('REQUEST_ID', REQUEST_ID); _libfop.php: $content['logID'] = REQUEST_ID; _libfop.php: $params['requestID'] = REQUEST_ID; _tasks/Hit.class.php: "logID" => REQUEST_ID, _testify/receiptUpload/testAll.php: $parameters['logID'] = REQUEST_ID; _testify/receiptUpload/testAll.php: $parameters['email'] = 'AMEX_'.REQUEST_ID; _testify/smartscan/autoScan.php: "logID" => REQUEST_ID, api/amex/v1/lib/net.class.php: $parameters['logID'] = REQUEST_ID; api/amex/v1/lib/net.class.php: $parameters['email'] = 'AMEX_'.REQUEST_ID; externalLib/amazon-sdk-2.3.2/Aws/AutoScaling/Enum/ScalingActivityStatusCode.php: const WAITING_FOR_SPOT_INSTANCE_REQUEST_ID = 'WaitingForSpotInstanceRequestId'; lib/Auth.php: $headers['requestID'] = REQUEST_ID; lib/Event.php: $safeRequestID = DB_Analytics::escapeWithoutConnection(REQUEST_ID); lib/Notification/Standard.php: $params['requestID'] = REQUEST_ID; lib/Report/Submit/Utils.php: TechnicalContactNotifier::addMessage('Request ID '.REQUEST_ID); Davids-MacBook-Pro:Web-Expensify dbarrett$

Alternatively, we use something like runkit_constant_redefine() to just redefine it when we fork(). This requires installing a custom package, however.

A third option would be to remove REQUEST_ID entirely... but I'm not sure really how to do that.

I think I lean toward (1) above. Thoughts?

Cc: @cead22

I agree that 1. seems like the best option. 2. has the drawbacks of requiring a package and maybe a lesser one that we're redefining a constant which can be confusing. As for 3. I don't even know what that would entail

iwiznia · 2017-04-13T10:32:08Z

src/Client.php

+                $this->sendRawRequest($rawRequest);
+                $response = $this->receiveRawResponse();
+            }
+            catch(ConnectionFailure $e)


Not sure I get this retry logic and the commands being marked as idempotent.
This catch, will catch ConnectionFailure errors thrown either in reconnect or in recv. The ones in reconnect I'm not worried about, but the ones in recv I think require additional logic. So, when it fails on recv there's a chance that bedrock did actually process the command, but we failed to get the response from that command back for whatever reason. If this is the case, then:

We can't retry a GetJob command (well, we can, but we'll get a different job back and the one that we got previously would be stuck in RUNNING state).

We can retry a finish/retry/fail command, but we need to handle the response differently in case of a retry, because when retried those commands will return an error 405 (since the job needs to be in RUNNING state).

Yes, but I think that's already accounted for by flagging which commands are idempotent:

GetJobs is kind of idempotent. Yes, if it fails, it'll leave a job RUNNING forever. But that's not BWM's problem -- all it's going to do is call GetJobs again. So retrying GetJobs is safe.

Something like QueryJob is fine to retry.

Even something like FinishJob or RetryJob are fine. Either they work right, or they return a "this has already been finished" error. Either way, there's no harm in retrying.

However, something like CreateJob cannot be safely retried, as you might be trying to create a job that executes exactly once, so retrying could cause it to happen twice (or more).

Ultimately, flag it as idempotent: true if it's safe to retry -- and in almost all cases, it is.

Even something like FinishJob or RetryJob are fine. Either they work right, or they return a "this has already been finished" error. Either way, there's no harm in retrying.

This is the part that I think is wrong. So, you are going to retry and it's going to respond with a "this has already been finished" error, when that happens, we need to treat that error as a success instead or BWM will try to mark the job as failed (and fail in doing so too)

iwiznia · 2017-04-13T10:33:18Z

src/Client.php

-        $this->sendRawRequest($rawRequest);
-        $response = $this->receiveRawResponse();
+        // Try idempotent requests up to three times, everything else only once
+        $numTries = (@$headers['idempotent'] === 'true') ? 3 : 1;


Why use "true" instead of true?

No idea why I did that. Fixing.

quinthar · 2017-04-13T23:38:13Z

Ready for re-review.

quinthar · 2017-04-15T00:08:30Z

src/Client.php

     * @param string          $writeConsistency  The bedrock write consistency we want to use
     *
     * @throws BedrockError
     */
-    public function __construct($host = null, $port = null, $failoverHost = null, $failoverPort = null, $connectionTimeout = null, $readTimeout = null, LoggerInterface $logger = null, StatsInterface $stats = null, $requestID = null, $writeConsistency = null)
+    public function __construct($host = null, $port = null, $failoverHost = null, $failoverPort = null, $connectionTimeout = null, $readTimeout = null, LoggerInterface $logger = null, StatsInterface $stats = null, $writeConsistency = null)


FYI, so far as I can tell nobody ever uses this constructor for anything but the defaults. My sense is this many attributes in the constructor is pretty excessive, especially since we have a static configure() function. I'd vote we just remove the constructor, or remove configure(). I'm not sure we're getting much value by having both.

PS: Why is this named Client rather than just the more obvious Bedrock? I understand the full class is Expensify\Bedrock\Client, and I even understand that other libraries (eg, Intercom) use the Client moniker. But it makes searching for it really hard because the fully-qualified name is rarely used:

Davids-MacBook-Pro:Web-Expensify dbarrett$ grep -i -r -s 'new client' * | grep -i bedrock | grep .php: | grep -v vendor/ | grep -v externalLib/ _devportal/tools/bedrockJobManager/api.php:$bedrock = new Client(); _devportal/tools/harvester/personalAutoHarvest.php: $bedrock = new Client(); _tasks/ParseResult.class.php: $bedrock = new Client(); _tasks/ScanningAPI.class.php: $bedrock = new Client(); _tests/integration/api/Policy/Connection/SetAutoSyncTest.php: $bedrock = new Client(); _tests/integration/api/Policy/Connection/SetAutoSyncTest.php: $bedrock = new Client(); _tests/integration/api/Policy/Connection/SetAutoSyncTest.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/ChatBotTestUtils.php: $bedrock = new Client(); _tests/integration/IntegrationTestUtils.php: $db = new \Expensify\Bedrock\DB(new Client()); _tests/integration/IntegrationTestUtils.php: $db = new \Expensify\Bedrock\DB(new Client()); _tests/integration/IntegrationTestUtils.php: $db = new \Expensify\Bedrock\DB(new Client()); _tests/integration/lib/Bedrock/JobsTest.php: self::$jobs = new Jobs(new Client()); _tests/integration/lib/Bedrock/JobsTest.php: $db = new DB(new Client()); _tests/integration/lib/Bedrock/JobsTest.php: $db = new DB(new Client()); _tests/integration/lib/Bedrock/JobsTest.php: $db = new DB(new Client()); _tests/integration/lib/Bedrock/ParentJobsTest.php: $jobs = new Jobs(new Client()); _tests/integration/lib/Bedrock/ParentJobsTest.php: $jobs = new Jobs(new Client()); _tests/integration/lib/Bedrock/ParentJobsTest.php: $jobs = new Jobs(new Client()); _tests/integration/lib/Bedrock/ParentJobsTest.php: $jobs = new Jobs(new Client()); _tests/integration/lib/Bedrock/ParentJobsTest.php: $jobs = new Jobs(new Client()); _tests/integration/lib/Bedrock/PriorityTest.php: $jobs = new Jobs(new Client()); _tests/integration/lib/ChatBot/APITest.php: $bedrock = new Client(); _tests/integration/lib/Tab/UtilsTest.php: self::$bedrock = new DB(new Client()); lib/ChatBot/ChatBot.php: $bedrock = new Client(); lib/ChatBot/Store/Chat.php: $bedrock = new Client(); lib/ChatBot/Store/Chat.php: $bedrock = new Client(); lib/ChatBot/Store/Event.php: $bedrock = new Client(); lib/ChatBot/Store/Event.php: $bedrock = new Client(); lib/ChatBot/Store/Event.php: $bedrock = new Client(); lib/ChatBot/Store/Input.php: $bedrock = new Client(); lib/ChatBot/Store/Input.php: $bedrock = new Client(); lib/ChatBot/Store/Input.php: $bedrock = new Client(); lib/ChatBot/Store/Input.php: $bedrock = new Client(); lib/ChatBot/Store/State.php: $bedrock = new Client(); lib/DomainSummary.php: $bedrock = new Client(); lib/Notification/Standard.php: $bedrock = new Client(); lib/Policy.php: $bedrock = new Client(); lib/Policy.php: $bedrock = new Client(); lib/PolicyAPI.php: $bedrock = new Client(); lib/Report/Reward.php: $bedrockJobs = new Jobs(new Client()); lib/ReportUtils.php: $bedrock = new Client(); script/bwm/EmailSendLogger.php: $bedrock = new Client(); script/bwm/PolicyHarvester.php: $bedrock = new Client(); script/bwm/PolicyHarvester.php: $bedrock = new Client(); script/bwm/PolicySummaryEmail.php: $bedrock = new Client(); script/bwm/UserHarvester.php: $bedrock = new Client(); script/createDomainWorkers.php:$bedrock = new Client(); Davids-MacBook-Pro:Web-Expensify dbarrett$

…nd reset it when forking)

quinthar · 2017-04-15T23:20:22Z

@iwiznia Ready for re-review

iwiznia · 2017-04-17T11:29:23Z

bin/BedrockWorkerManager.php

-    throw new Exception('Maximum load must be greater than zero');
+// Parse the command line and verify the required settings are provided
+$options = getopt('', ['host::', 'port::', 'failoverHost::', 'failoverPort::', 'maxLoad::', 'maxIterations::', 'jobName::', 'logger::', 'stats::', 'workerPath::', 'versionWatchFile::', 'writeConsistency::']);
+$workerPath = @$options['workerPath'];


$options['workerPath'] ?? null

Per offline discussion, this is NAB

iwiznia · 2017-04-17T11:30:06Z

bin/BedrockWorkerManager.php

-Client::configure($bedrockConfig);
+// Add defaults
+$jobName = $options['jobName'] ?? '*'; // Process all jobs by default
+$maxLoad = floatval(@$options['maxLoad']) ?: 1.0; // Max load of 1.0 by default


floatval($options['maxLoad'] ?? 1.0)

Per offline discussion, this is NAB

Actually... on this does the ?? just implicitly do the error suppression on the left expression? So it'll work even if inside a function? That's pretty slick.

$options['maxLoad'] ?? 1.0 is equivalent to (isset($options['maxLoad']) ? $options['maxLoad'] : 1.0)

iwiznia · 2017-04-17T11:30:27Z

bin/BedrockWorkerManager.php

+// Add defaults
+$jobName = $options['jobName'] ?? '*'; // Process all jobs by default
+$maxLoad = floatval(@$options['maxLoad']) ?: 1.0; // Max load of 1.0 by default
+$maxIterations = intval(@$options['maxIterations']) ?: -1; // Unlimited iterations by default


intval($options['maxIterations'] ?? -1)

iwiznia · 2017-04-17T11:30:53Z

bin/BedrockWorkerManager.php

+$logger->info('Starting BedrockWorkerManager', ['maxIterations' => $maxIterations]);
+
+// If --versionWatch is enabled, begin watching a version file for changes
+$versionWatchFile = @$options['versionWatchFile'];


$options['versionWatchFile'] ?? null

Per offline discussion, this is NAB

iwiznia · 2017-04-17T11:36:24Z

src/Client.php

-        $this->sendRawRequest($rawRequest);
-        $response = $this->receiveRawResponse();
+        // Try idempotent requests up to three times, everything else only once
+        $numTries = @$headers['idempotent'] ? 3 : 1;


isset($headers['idempotent']) ? 3 : 1

iwiznia · 2017-04-17T11:36:52Z

src/Client.php

+                $this->sendRawRequest($rawRequest);
+                $response = $this->receiveRawResponse();
+            }
+            catch(ConnectionFailure $e) {


NAB: this should be on the same line as the } above.

iwiznia · 2017-04-17T11:38:54Z

src/Client.php

+        // Try idempotent requests up to three times, everything else only once
+        $numTries = @$headers['idempotent'] ? 3 : 1;
+        $response = null;
+        while($numTries-- && !$response) {


This comment wasn't answered.

iwiznia · 2017-04-17T11:50:24Z

bin/BedrockWorkerManager.php

+                        // REQUEST_IDs.
+                        mt_srand(getmypid());
+                        $GLOBALS['REQUEST_ID'] = substr(base64_encode(sha1(mt_rand())),0,6); // random 6 character ID
+                        $logger->info("Fork succeeded, child process, running job", [


Can you change this text a bit so it's easier to search for parent vs child?

Comment cleanup, shutdown logic tweaks

392321f

quinthar commented Apr 12, 2017

View reviewed changes

More cleanup; added a super basic test framework

fa2df58

quinthar commented Apr 13, 2017

View reviewed changes

quinthar requested a review from cead22 April 13, 2017 01:04

quinthar self-assigned this Apr 13, 2017

Added Client::setGlobalRequestID()

49566ce

quinthar changed the title ~~[HOLD] Comment cleanup, shutdown logic tweaks~~ Add simple test framework for Bedrock::Jobs Apr 13, 2017

quinthar changed the title ~~Add simple test framework for Bedrock::Jobs~~ Add simple test framework for Bedrock::Jobs; add requestID Apr 13, 2017

quinthar requested review from iwiznia and removed request for cead22 April 13, 2017 06:11

quinthar added 5 commits April 12, 2017 23:17

Bumped the composer version

04033bc

Removed debug code

ac567b2

Fixed for setGlobalRequestID()

d9421ab

Moved public function up by other public functions

6dcbccd

Added retrying of idempotent requests that fail after sending

cc5c8ac

quinthar changed the title ~~Add simple test framework for Bedrock::Jobs; add requestID~~ Add simple test framework for Bedrock::Jobs; add requestID; add retries Apr 13, 2017

Updated usage docs

4bf8128

quinthar commented Apr 13, 2017

View reviewed changes

Comment fixes

8d3af58

iwiznia requested changes Apr 13, 2017

View reviewed changes

quinthar added 3 commits April 13, 2017 11:03

Removed dependency on Log::.

8984f3e

Fixed stupid "true"

d73e280

Fixed bracket style

e2391f8

quinthar added 2 commits April 14, 2017 10:09

Upgrading warn to error per PR review

105ffb2

Consolidate on global REQUEST_ID constant

9e6bcbf

quinthar commented Apr 15, 2017

View reviewed changes

Replace global REQUEST_ID constant with global REQUEST_ID variable (a…

6617274

…nd reset it when forking)

quinthar changed the title ~~Add simple test framework for Bedrock::Jobs; add requestID; add retries~~ Add simple demo framework for Bedrock::Jobs; add requestID; add retries Apr 15, 2017

Renamed test.sh to demo.sh and made more minimal

8f42bd9

iwiznia requested changes Apr 17, 2017

View reviewed changes

quinthar added 2 commits April 17, 2017 11:12

Attempt to suppress benign errors

b005866

Fixed style

4433ef1

iwiznia approved these changes Apr 17, 2017

View reviewed changes

iwiznia merged commit 52ba40d into master Apr 17, 2017

iwiznia deleted the dbarrett_requestIDTweaks branch April 17, 2017 18:40


		// Configure the Bedrock client with these command-line options
		Client::configure($options);

Add simple demo framework for Bedrock::Jobs; add requestID; add retries #15

Add simple demo framework for Bedrock::Jobs; add requestID; add retries #15

Conversation

quinthar commented Apr 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinthar Apr 13, 2017 • edited

Choose a reason for hiding this comment

quinthar Apr 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinthar Apr 14, 2017 • edited

Choose a reason for hiding this comment

How REQUEST_ID works today:

What this PR does:

Proposing a better solution:

cead22 Apr 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinthar commented Apr 13, 2017

quinthar Apr 15, 2017 • edited

Choose a reason for hiding this comment

quinthar commented Apr 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwiznia Apr 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinthar commented Apr 12, 2017 •

edited

quinthar Apr 13, 2017 •

edited

quinthar Apr 13, 2017 •

edited

quinthar Apr 14, 2017 •

edited

How `REQUEST_ID` works today:

cead22 Apr 14, 2017 •

edited

quinthar Apr 15, 2017 •

edited

iwiznia Apr 17, 2017 •

edited