{169607487} feat: enable distributed trace addon by default#5709
{169607487} feat: enable distributed trace addon by default#5709SChakravorti21 merged 1 commit intobloomberg:mainfrom
Conversation
SChakravorti21
force-pushed
the
port-dt
branch
4 times, most recently
from
228ec92 to
ce37fc5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/plugin-branch port-dt |
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Coding style check: Success ✓.
Cbuild submission: Error ⚠.
Regression testing: 0/0 tests failed ⚠.
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Coding style check: Success ✓.
Cbuild submission: Error ⚠.
Regression testing: 0/0 tests failed ⚠.
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Coding style check: Success ✓.
Cbuild submission: Success ✓.
Regression testing: 0/0 tests failed ⚠.
SChakravorti21
force-pushed
the
port-dt
branch
2 times, most recently
from
5d5402d to
33fa914
Compare
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Coding style check: Success ✓.
Cbuild submission: Success ✓.
Regression testing: 0/0 tests failed ⚠.
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Coding style check: Success ✓.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
sirace [setup failure]
catchup_idle [setup failure]
truncatesc [setup failure]
maxtable [setup failure]
cinsert_linearizable [setup failure]
socksql_master_swings [setup failure]
systable_locking [setup failure]
verify_writes [setup failure]
sc_force [setup failure]
truncatesc_offline_generated [setup failure]
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Coding style check: Success ✓.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated
api_dl_libs
insert_lots_ssl_generated
insert_lots
SChakravorti21
force-pushed
the
port-dt
branch
4 times, most recently
from
d5749c2 to
6692070
Compare
| * so users still have a way to disable it, if so desired. */ | ||
| if (cdb2_install != NULL) { | ||
| pthread_mutex_lock(&cdb2_sockpool_mutex); | ||
| (*cdb2_install)("dt"); |
There was a problem hiding this comment.
Compile this conditionally under COMDB2_BBCMAKE? what do you think.
I would probably introduce a compile time option, say, COMDB2_INIT_ADDONS, pass it here, and define it to dt in builddb.
There was a problem hiding this comment.
Yes I was thinking something similar, like cdb2_install_default_libs that is only defined in BB build. Then this code would become:
if (cdb2_install_default_libs != NULL) {
pthread_mutex_lock(&cdb2_sockpool_mutex);
(*cdb2_install_default_libs)();
pthread_mutex_unlock(&cdb2_sockpool_mutex);
}Is this along the lines of what you're thinking?
There was a problem hiding this comment.
Or do you mean just defining the list of addons as a compile time flag? Like a CMake flag -DCOMDB2_INIT_ADDONS="dt".
There was a problem hiding this comment.
I figured the second one, for default addons
There was a problem hiding this comment.
I like that idea actually
There was a problem hiding this comment.
#ifdef CDB2_INIT_ADDONS
(*cdb2_install)(STRINGIFY(CDB2_INIT_ADDONS));
#endifAnd in builddb (or however Salil has decided to build it internally), we define CDB2_INIT_ADDONS to dt.
I think it makes code clear if we hide dt from open source.
There was a problem hiding this comment.
That looks good to me! Going to merge this PR for now since there's multiple follow-up items and I don't want the two APIs to diverge. Will address in a follow-up PR.
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
Enable the (Bloomberg-internal) distributed trace plugin by default so that users get it out of the box. For open source builds of cdb2api, this is a no-op. Additionally, provide a way to disable this behavior. We already have the ability to do so through an environment variable (`COMDB2_CONFIG_UNINSTALL_STATIC_LIBS=dt`) or using the config file (`comdb2_config:uninstall_static_libs_v4=dt`), but both only accept _one_ addon name. If we want to enable more plugins by default in the future, users should be able to selectively disable them as well. Support supplying a list of plugin names for both the environment variable and the config file setting. Signed-off-by: Shoumyo Chakravorti <schakravorti@bloomberg.net>
SChakravorti21
force-pushed
the
port-dt
branch
from
6692070 to
0dcf6b1
Compare
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Error ⚠.
Regression testing: Success ✓.
The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated
reco-ddlk-sql
roborivers
left a comment
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
consumer_non_atomic_default_consumer_generated
4 participants
Enable the (Bloomberg-internal) distributed trace plugin by default so that users get it out of the box. For open source builds of cdb2api, this is a no-op.
Additionally, provide a way to disable this behavior. We already have the ability to do so through an environment variable
(
COMDB2_CONFIG_UNINSTALL_STATIC_LIBS=dt) or using the config file (comdb2_config:uninstall_static_libs_v4=dt), but both only accept one addon name. If we want to enable more plugins by default in the future, users should be able to selectively disable them as well. Support supplying a list of plugin names for both the environment variable and the config file setting.