Skip to content

SQL: [Tests] Fix and enable internalClusterTests#37300

Merged
matriv merged 10 commits intoelastic:masterfrom
matriv:mt/fix-37191
Jan 11, 2019
Merged

SQL: [Tests] Fix and enable internalClusterTests#37300
matriv merged 10 commits intoelastic:masterfrom
matriv:mt/fix-37191

Conversation

@matriv
Copy link
Copy Markdown
Contributor

@matriv matriv commented Jan 10, 2019

Fixes: #37191

@matriv matriv added >test-failure Triaged test failures from CI v7.0.0 :Analytics/SQL SQL querying v6.7.0 labels Jan 10, 2019
@matriv matriv requested review from alpar-t, astefan and costin January 10, 2019 12:55
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search


import java.nio.file.Path;

public class LocalStateSQLXpackPlugin extends LocalStateCompositeXPackPlugin {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it have to be public? Also, I think the naming around xpack should be XPack (capital P).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, can be package private, thx!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, it needs to be public:

   > Throwable #1: java.lang.IllegalStateException: no public constructor for [org.elasticsearch.xpack.sql.action.LocalStateSQLXPackPlugin]
   >    at org.elasticsearch.plugins.PluginsService.loadPlugin(PluginsService.java:590)
   >    at org.elasticsearch.plugins.PluginsService.<init>(PluginsService.java:119)
   >    at org.elasticsearch.node.Node.<init>(Node.java:303)

Copy link
Copy Markdown
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left one comment.

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 10, 2019

run default distro tests

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 10, 2019

run gradle build tests 2

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 10, 2019

run default distro tests

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 10, 2019

run gradle build tests 2

@matriv matriv added the WIP label Jan 10, 2019
@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 10, 2019

Seems that the changes affect the integTestCluster, so Marked it as WIP for the moment.

SqlPlugin cannot have more than one constructor, so for the testing
purposes one should set a dummy `sqlLicenseChecker` instead.
@matriv matriv removed the WIP label Jan 10, 2019
@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 10, 2019

With the awesome help of @hub-cap the issue has been fixed.
The PR is ready for review.

Also opened a new issue: #37320 to fix the licencing tests.

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 11, 2019

@astefan Maybe you want to also take another look here.

Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM.

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 11, 2019

run gradle build tests 2

@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 11, 2019

run gradle build tests 1

@matriv matriv merged commit 85531f0 into elastic:master Jan 11, 2019
@matriv matriv deleted the mt/fix-37191 branch January 11, 2019 20:43
@astefan
Copy link
Copy Markdown
Contributor

astefan commented Jan 11, 2019

@matriv still LGTM

matriv added a commit that referenced this pull request Jan 11, 2019
SqlPlugin cannot have more than one public constructor, so for the testing
purposes the `getLicenseState()` should be overriden.

Fixes: #37191

Co-authored-by: Michael Basnight <mbasnight@gmail.com>
@matriv
Copy link
Copy Markdown
Contributor Author

matriv commented Jan 11, 2019

Backported to 6.x with 5e17a78

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 12, 2019
…rsistence

* elastic/master:
  Fix PrimaryAllocationIT Race Condition (elastic#37355)
  SQL: Make `FULL` non-reserved keyword in the grammar (elastic#37377)
  SQL: [Tests] Fix and enable internalClusterTests (elastic#37300)
  ML: Fix testMigrateConfigs  (elastic#37373)
  Fix RollupDocumentation test to wait for job to stop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/SQL SQL querying >test-failure Triaged test failures from CI v6.7.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants