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

MDBF-535: add spider to protected branches #101

Closed
wants to merge 1 commit into from

Conversation

grooverdan
Copy link
Member

No description provided.

Copy link
Collaborator

@vladbogo vladbogo left a comment

Choose a reason for hiding this comment

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

  1. I think this is not enough since the compilation disable SPIDER (-DPLUGIN_SPIDER=NO)
  2. Do we need to do some sort of announcement before adding this to prod?

@grooverdan
Copy link
Member Author

I think this is not enough since the compilation disable SPIDER (-DPLUGIN_SPIDER=NO)

Quite right, all protected branches are f_quick_build. Fix added.

Do we need to do some sort of announcement before adding this to prod?

Yuchen and I looked over a lot of the potential existing failures in spider and found it pretty clear of mtr failures. Rather than announce this one and get a number of new extensions I proposed, we increase running of existing mtr test cases slowly in protected branches so eventually unintended regressions are positively discovered.

Copy link
Collaborator

@vladbogo vladbogo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -18,7 +18,7 @@ def getQuickBuildFactory(mtrDbPool):
f_quick_build.addStep(steps.ShellCommand(name="create html log file", command=['bash', '-c', util.Interpolate(getHTMLLogString(), jobs=util.Property('jobs', default='$(getconf _NPROCESSORS_ONLN)'))]))
# build steps
f_quick_build.addStep(steps.Compile(command=
["sh", "-c", util.Interpolate("cmake . -DCMAKE_BUILD_TYPE=%(kw:build_type)s -DCMAKE_C_COMPILER_LAUNCHER=ccache -DCMAKE_C_COMPILER=%(kw:c_compiler)s -DCMAKE_CXX_COMPILER_LAUNCHER=ccache -DCMAKE_CXX_COMPILER=%(kw:cxx_compiler)s -DPLUGIN_TOKUDB=NO -DPLUGIN_MROONGA=NO -DPLUGIN_SPIDER=NO -DPLUGIN_OQGRAPH=NO -DPLUGIN_PERFSCHEMA=%(kw:perf_schema)s -DPLUGIN_SPHINX=NO %(kw:additional_args)s && make -j%(kw:jobs)s %(kw:create_package)s", perf_schema=util.Property('perf_schema', default='YES'), build_type=util.Property('build_type', default='RelWithDebInfo'), jobs=util.Property('jobs', default='$(getconf _NPROCESSORS_ONLN)'), c_compiler=util.Property('c_compiler', default='gcc'), cxx_compiler=util.Property('cxx_compiler', default='g++'), additional_args=util.Property('additional_args', default=''), create_package=util.Property('create_package', default='package') )], env={'CCACHE_DIR':'/mnt/ccache'}, haltOnFailure="true"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use additional_args in order not to enable spider everywhere? Or is it ok to be enabled? Other than that looks good.

@fauust fauust force-pushed the main branch 3 times, most recently from 87ffbbf to c75508d Compare March 2, 2023 14:43
@vladbogo
Copy link
Collaborator

@grooverdan in order to merge this, should we use additional_args in order not to enable spider everywhere? Or is it ok to be enabled? Other than that looks good.

Adding spider to the build option enables this for all quick workers.

The mtr by default include the set of stable spider tests so a full
enable is expected.

For the protected branch, explictly list the spider suites.
Of note, in 10.11 the spider/handler was removed and in 10.3 there
is no spider/regression or spider/feature tests.
@mariadb-YuchenPei
Copy link
Contributor

2cdc13d LGTM

@grooverdan
Copy link
Member Author

@grooverdan in order to merge this, should we use additional_args in order not to enable spider everywhere? Or is it ok to be enabled? Other than that looks good.

Looking closer, planned to enable it everywhere. Time it as 2-3mins on 16 mtr workers.

@vladbogo
Copy link
Collaborator

Closed in favour of #145

@vladbogo vladbogo closed this Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants