-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[dev/cli] detect worker type using env, not cluster module #83977
Changes from all commits
a883ede
2d034f5
60b1a19
6347688
6b67a1d
32f9e7c
b5446b7
6f72a36
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,6 @@ | |
*/ | ||
|
||
import * as chokidar from 'chokidar'; | ||
import { isMaster } from 'cluster'; | ||
import fs from 'fs'; | ||
import { Server } from '@hapi/hapi'; | ||
import { throttle } from 'lodash'; | ||
|
@@ -351,22 +350,14 @@ export class LogRotator { | |
} | ||
|
||
_sendReloadLogConfigSignal() { | ||
if (isMaster) { | ||
(process as NodeJS.EventEmitter).emit('SIGHUP'); | ||
if (!process.env.isDevCliChild || !process.send) { | ||
process.emit('SIGHUP', 'SIGHUP'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are there two arguments to this function? It appears the second argument is supposed to be a socket or server? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what |
||
return; | ||
} | ||
|
||
// Send a special message to the cluster manager | ||
// so it can forward it correctly | ||
// It will only run when we are under cluster mode (not under a production environment) | ||
if (!process.send) { | ||
this.log( | ||
['error', 'logging:rotate'], | ||
'For some unknown reason process.send is not defined, the rotation was not successful' | ||
); | ||
return; | ||
} | ||
|
||
process.send(['RELOAD_LOGGING_CONFIG_FROM_SERVER_WORKER']); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,10 +30,6 @@ let apmConfig; | |
const isKibanaDistributable = Boolean(build && build.distributable === true); | ||
|
||
module.exports = function (serviceName = name) { | ||
if (process.env.kbnWorkerType === 'optmzr') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to start APM for both parent and child CLI process? Do they behave differently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if we want to run APM in both processes, but they do behave differently. The parent process is basically just loading enough of Kibana to understand the config files and proxy requests to the child process which runs the full server and is restarted by the parent process when a server file is changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the way things are setup it looks like APM is intentionally setup in the parent process, and it uses a different service name to identify itself as the parent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the info, yeah makes sense. I am fine with keeping it as it is for now. |
||
return; | ||
} | ||
|
||
apmConfig = loadConfiguration(process.argv, ROOT_DIR, isKibanaDistributable); | ||
const conf = apmConfig.getConfig(serviceName); | ||
const apm = require('elastic-apm-node'); | ||
|
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.
In master all workers are of type server, so this condition will always evaluate to false