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
HPCC-28968 Expose internal HPCC settings as environment variables #17012
HPCC-28968 Expose internal HPCC settings as environment variables #17012
Conversation
37eeda3
to
2cf410e
Compare
@ghalliday - this is new PR for closed #17010 (couldn't reopn). Only difference is fixed comment typo' as per your comment in that PR. |
https://track.hpccsystems.com/browse/HPCC-28968 |
@ghalliday - added extra commit to fix win32 compile issue. |
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.
@jakesmith unfortunately not thread safe.
system/jlib/jmisc.cpp
Outdated
queryEnvironmentConf().getProp("deploymentName", deploymentName); | ||
if (0 == deploymentName.length()) | ||
deploymentName.set("UNKNOWN"); | ||
setenv("HPCC_DEPLOYMENT", deploymentName, 1); |
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.
unfortunately setenv isn't thread-safe. That means that changes coming in here could cause the program to crash.... Needs some more thought. Probably our own structure (the config/global environment?) with thread protection.
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.
ok, the original idea was to hook getEnv, so that it special cased based on some convention, and got the value from our own internal env. section.
Pity, but I'll change to follow that route vs using standard process env. variables.
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.
@ghalliday - please see revised scheme.
system/jlib/jptree.cpp
Outdated
unsigned notifyFuncId = pendingInitializeFuncIds.back(); | ||
pendingInitializeFuncIds.pop_back(); | ||
ConfigUpdateFunc notifyFunc = notifyConfigUpdates[notifyFuncId]; | ||
notifyFunc(getComponentConfigSP(), getGlobalConfigSP()); |
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.
trivial: more efficient to evaluate these once outside the loop? Fine as it is.
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.
I'll change it.
da5bd1a
to
88db8c4
Compare
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.
A couple of minor comments. Please squash (and fix the spurious newline)
thorlcr/thorcodectx/thcodectx.cpp
Outdated
@@ -1,3 +1,4 @@ | |||
|
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.
spurious newline
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.
will remove.
ecl/eclagent/eclagent.cpp
Outdated
char *hpccEnvVal = getHPCCEnvVal(name, defaultValue); | ||
if (hpccEnvVal) | ||
return hpccEnvVal; | ||
else |
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.
picky: I think this else is confusing - when I first read it I assumed the true branch would fall through to the code below, and then leak because of the extra strdup()
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.
agree, I wouldn't normally write like that, not sure why I did, will change.
88db8c4
to
9decf7b
Compare
In particular the deployment name, such that ECL can use it with getEnv("HPCC_DEPLOYMENT") Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>
9decf7b
to
6621919
Compare
@ghalliday - minor changes made, and squashed |
In particular the deployment name, such that ECL can use it with getEnv("HPCC_DEPLOYMENT")
Type of change:
Checklist:
Smoketest:
Testing: