-
Notifications
You must be signed in to change notification settings - Fork 3.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
HBASE-27055 Add additional comments when using HBASE_TRACE_OPTS with standalone mode #4452
Conversation
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
bin/hbase
Outdated
@@ -508,7 +508,7 @@ add_opentelemetry_agent() { | |||
fi | |||
agent_jar=$(tr ':' '\n' < "${f}" | grep opentelemetry-javaagent) | |||
fi | |||
HBASE_OPTS="$HBASE_OPTS -javaagent:$agent_jar" | |||
HBASE_OPTS="$HBASE_TRACE_OPTS -javaagent:$agent_jar" |
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.
Good catch!
I don't think this correct either. Shouldn't we be adding HBASE_TRACE_OPTS
to HBASE_OPTS
, not overriding HBASE_OPTS
?
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.
Maybe instead we need an if-block specifically for standalone mode?
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.
oops, you're right, I was just testing on standalone mode and forgot we may have more in HBASE_OPTS
Reading back over the patch on #3762. Maybe the fix is to require the operator to assign What do you think? |
Something like,
|
Actually, site-wide is not sufficient for any multi-process deployment. In most cases, the operator really should use the per-process options so that they can provide proper service names in trace metadata. |
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 think it's better to leave this up to the operator to configure explicitly in conf/hbase-env.sh
. Alternatively, you'll need to re-work how we handle per-process configuration.
I got the comment for the conf/hbase-env.sh, but I need to spend some time to think if the current per-processes has anything wrong.... |
I rethink about your comment on per-processor configuration, the current per-process configuration should be the best solution. I was from the standalone testing environment, and that's minor to the overall configuration. such I changed the subject of this PR to add additional comment when using HBASE_TRACE_OPTS with standalone mode, that should solve the confusion as an user for this environment parameters. or you can say this task is not necessary, we can just close it. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
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.
Yep, looks right to me. Thanks for noticing this one!
…standalone mode (#4452) Signed-off-by: Nick Dimiduk <[email protected]>
…standalone mode (#4452) Signed-off-by: Nick Dimiduk <[email protected]>
…standalone mode (apache#4452) Signed-off-by: Nick Dimiduk <[email protected]>
…TS with standalone mode (apache#4452)" This reverts commit 445573e.
tested locally and now standalone with these configuration
export HBASE_TRACE_OPTS="-Dotel.traces.exporter=jaegar -Dotel.metrics.exporter=none"
inconf/hbase-env.sh
no longer failedbefore the change, it's always using the default and create Otlp metrics exporter but it cannot be done.