Skip to content
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

Unify daemon processes #848

Merged
merged 46 commits into from
Feb 8, 2018
Merged

Unify daemon processes #848

merged 46 commits into from
Feb 8, 2018

Conversation

chingor13
Copy link
Contributor

@chingor13 chingor13 commented Jan 13, 2018

The BatchDaemon is great for submitting items in the background. We can abstract the concept of a job and leverage the job runner framework to support other job types - the primary example is supporting the Debugger daemon - a simple loop that uses long polling for results.

Currently, to use the debugger, you need to set up 2 daemon processes. By combining the daemons, the user only needs to set up a single daemon.

In this PR:

  • Create new JobInterface
  • BatchConfig is now JobConfig and stores JobInterface instances
  • Introduce SimpleJob which is used for jobs with a single blocking method that runs in a loop.
  • Fix a few method names in the Batch namespace to match our getter/setter naming convention.
  • Debugger Daemon is now a simple job type and is registered when the first Agent is created - this removes the need for a separate daemon process

Example custom simple job:

class MySimpleJob
{
    use SimpleJobTrait;

    public function __construct()
    {
        $this->setSimpleJobProperties([
            'identifier' => 'my-simple-job'
        ]);
    }

    public function run()
    {
        while (true) {
            echo "looping\n";
            sleep(2);
        }
    }
}

@chingor13 chingor13 requested a review from tmatsuo as a code owner January 13, 2018 00:18
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 13, 2018
@chingor13
Copy link
Contributor Author

@jdpedrie @dwsupplee This is not ready for a full review, but does abstracting the job interface make sense to you? It allows for configuring other asynchronous jobs besides batch jobs (with an item message queue). The primary use case for now is to handle the debugger's breakpoint fetching loop (which currently must be run as its own separate daemon).

@dwsupplee
Copy link
Contributor

@chingor13, sounds like a great direction to move towards. 👍

@chingor13
Copy link
Contributor Author

@dwsupplee @jdpedrie @tmatsuo This should be ready for a look.

$job->run($items);
$this->assertEquals($expected, $this->items);
}
// public function testRun()

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@tmatsuo
Copy link
Contributor

tmatsuo commented Feb 6, 2018

I tried this branch on GAE Flex.

The code below

$batchLogger = LoggingClient::psrBatchLogger(
    'app',
    ['debugOutput' => false,
     'batchOptions' => ['workerNum' => 8],
     'clientConfig' => [
         'projectId' => getenv('GOOGLE_CLOUD_PROJECT'),
         'transport' => 'rest'
     ]
    ]
);

only spun up 2 child processes. Does it ring a bell?

@chingor13
Copy link
Contributor Author

I had changed the parameter to numWorkers to be more clear. workerNum seems like it's the worker id number. We can change it back if we're concerned about breaking people who may have been using this feature.

@tmatsuo
Copy link
Contributor

tmatsuo commented Feb 6, 2018

That's definitely it. Perfonally I'm fine breaking this, but I'll defer to @dwsupplee for decision.

The performance wise, the change has no problems at all.

@chingor13 chingor13 requested review from dwsupplee and removed request for dwsupplee February 7, 2018 20:11
Copy link
Contributor

@dwsupplee dwsupplee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nicely thought out improvement. Great work.

numWorkers sounds good to me. We have marked the batch feature as experimental everywhere so we have the leeway to make these kinds of improvements.

A few items:

{
/**
* @var BatchJob[]
* @var array[string]JobInterface Associative array of JobInterface instances

This comment was marked as spam.

@@ -0,0 +1,86 @@
<?php
/**
* Copyright 2017 Google Inc. All Rights Reserved.

This comment was marked as spam.

@@ -207,6 +246,12 @@ private function defaultSourceContext()
return [];
}


This comment was marked as spam.

* to** a new DebuggerClient.
* @type array $extSourceContext The source code identifier. **Defaults
* @type string $sourceRoot The full path to the source root
* @type array $clientOptions The options to instantiate the default

This comment was marked as spam.

This comment was marked as spam.

@chingor13
Copy link
Contributor Author

I'll add some more tests for the new job classes/traits and will update to use the closure serializer.

@chingor13
Copy link
Contributor Author

Hold off on merging this. The system test is failing and I'm looking into it.

@chingor13
Copy link
Contributor Author

@dwsupplee The system test is fixed. The actual problem was from #887 where I had made BreakpointStorageInterface#load always return data rather than null/[] if the debuggee is not yet registered. This caused a timing issue because we started the breakpoint reporter with a null debuggeeId so the agent could not successfully send breakpoint data. The fix was to not start the breakpoint reporter agent until we receive a debuggeeId indicating that the debuggee has been registered.

@dwsupplee dwsupplee merged commit 4f79a5e into googleapis:master Feb 8, 2018
@chingor13 chingor13 deleted the single-daemon branch February 8, 2018 23:51
@jdpedrie jdpedrie mentioned this pull request Feb 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants