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

HLRC XPack Protocol clean up: Migration; Graph; Watcher #34639

Merged

Conversation

vladimirdolzhenko
Copy link
Contributor

Move parts related to XPack for packages Migration; Graph; Watcher from protocol to client

Drop protocol module

Relates #34451

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra


public class IndexUpgradeInfoResponseTests extends AbstractStreamableXContentTestCase<IndexUpgradeInfoResponse> {
@Override
protected IndexUpgradeInfoResponse doParseInstance(XContentParser parser) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep these tests somehow so we're fairly confident of the parsing? Not, like, the tests how they look now, but some kind of parsing tests? Like we've done for the rollup response stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nik9000 we can keep it ToXContentObject in this PR and keep this test as-is, in #34547 I added cross xpack-hlrc compatibility tests that makes ToXContentObject for hlrc objects a bit of redundant

Copy link
Member

Choose a reason for hiding this comment

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

👍

@vladimirdolzhenko vladimirdolzhenko changed the base branch from hlrc_xprotocol_cleanup_1 to master October 19, 2018 17:21
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

In combination with the other in flight PRs around testing, this looks good to me.

import org.elasticsearch.test.ESTestCase;

public class IndexUpgradeInfoResponseTests extends ESTestCase {
// TODO: add to cross XPack-HLRC serialization test
Copy link
Member

Choose a reason for hiding this comment

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

👍 I see you already have that in flight in another PR so this is fine by me!

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

Great work. @rjernst will be jealous that you got to remove so much code instead of him ;) One small nit on 2 watcher classes but i dont need to see this again.

@@ -51,7 +51,6 @@ dependencies {
compile "org.elasticsearch.plugin:aggs-matrix-stats-client:${version}"
compile "org.elasticsearch.plugin:rank-eval-client:${version}"
compile "org.elasticsearch.plugin:lang-mustache-client:${version}"
bundle project(':x-pack:protocol')
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

ActionRequestValidationException validationException = null;
if (id == null){
validationException = ValidateActions.addValidationError("watch id is missing", validationException);
public Optional<ValidationException> validate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -136,20 +109,20 @@ public void setVersion(long version) {
}

@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException validationException = null;
public Optional<ValidationException> validate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@nik9000
Copy link
Member

nik9000 commented Oct 26, 2018

❤️

@vladimirdolzhenko vladimirdolzhenko merged commit 69fe9a1 into elastic:master Oct 29, 2018
vladimirdolzhenko added a commit to vladimirdolzhenko/elasticsearch that referenced this pull request Oct 29, 2018
Move parts related to XPack for packages Migration; Graph; Watcher from protocol to client

Drop protocol module

Relates elastic#34451, backport of elastic#34639

(cherry picked from commit 69fe9a1)
vladimirdolzhenko added a commit that referenced this pull request Oct 29, 2018
Move parts related to XPack for packages Migration; Graph; Watcher from protocol to client

Drop protocol module

Relates #34451, backport of #34639

(cherry picked from commit 69fe9a1)
@vladimirdolzhenko vladimirdolzhenko deleted the hlrc_xprotocol_cleanup_2 branch October 29, 2018 20:56
kcm pushed a commit that referenced this pull request Oct 30, 2018
Move parts related to XPack for packages Migration; Graph; Watcher from protocol to client

Drop protocol module

Relates #34451
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants