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.

Sign up for GitHub

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

Jump to bottom

Fix toString of class SnapshotStatus (#26851) #26852

Merged
merged 1 commit into from
Oct 5, 2017
Merged

Fix toString of class SnapshotStatus (#26851) #26852

merged 1 commit into from
Oct 5, 2017

Conversation

liketic
Copy link

@liketic liketic commented Oct 2, 2017

Fixes #26851

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

cbuescher
cbuescher reviewed Oct 2, 2017
View reviewed changes
core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatus.java Outdated
@@ -162,9 +161,7 @@ public static SnapshotStatus readSnapshotStatus(StreamInput in) throws IOExcepti
public String toString() {
try {
XContentBuilder builder = XContentFactory.jsonBuilder().prettyPrint();
builder.startObject();
Copy link
Member

Choose a reason for hiding this comment

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

There's utility method that could replace all of this in org.elasticsearch.common.String. I think toString(ToXContent toXContent, boolean pretty, boolean human) can replace all of this methods code.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for reviewing this.

cbuescher
cbuescher requested changes Oct 2, 2017
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

Hi @liketic, I left a short suggestion about making the toString() method shorter.

@cbuescher cbuescher added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v7.0.0 labels Oct 2, 2017
@cbuescher cbuescher self-assigned this Oct 3, 2017
cbuescher
cbuescher reviewed Oct 3, 2017
View reviewed changes
core/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatus.java Outdated
} catch (IOException e) {
return "{ \"error\" : \"" + e.getMessage() + "\"}";
}
return Strings.toString(this, true, true);
Copy link
Member

Choose a reason for hiding this comment

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

Since the original toString() method only used the pretty printing option but not the 'human` option, could we keep that the same?

Copy link
Author

Choose a reason for hiding this comment

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

Make sense. Thanks.

cbuescher
cbuescher approved these changes Oct 4, 2017
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@liketic looks good to me now, thanks again for picking this up. I will merge this to master and the 6.1 branch.

@cbuescher
Copy link
Member

@elasticmachine test this please

cbuescher
cbuescher reviewed Oct 4, 2017
View reviewed changes
...c/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatusTests.java
@@ -0,0 +1,89 @@
package org.elasticsearch.action.admin.cluster.snapshots.status;
Copy link
Member

Choose a reason for hiding this comment

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

The CI tests just complained about missing license headers for this new test file. Can you please copy over the header comments from any existing test to this one? Sorry I missed this during review.

@cbuescher
Copy link
Member

 @liketic sorry I missed it before, can you add our standard license headers to the new test file?

@cbuescher
Copy link
Member

@elasticmachine Test this please

cbuescher
cbuescher requested changes Oct 4, 2017
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

The new SnapshotStatusTests is still failing. Depending on which shardStage is picked, the output is different. Could you update the test to fix this? I think the randomization of the shardStage is a thing we want to keep.

@cbuescher
Copy link
Member

@liketic thanks, I'll start another CI run
@elasticmachine test this please

@liketic
Copy link
Author

liketic commented Oct 5, 2017 •
edited
Loading

No idea about why the testing is still failed... I can get success on my laptop but only sometimes...

@cbuescher
Copy link
Member

@liketic those failures look unrelated, however I'd like to have a clean build before merging this. Sometimes its the commit you branched from, would you mind rebasing this PR or merging in master again so we can see if we get a clean CI run?

@liketic liketic force-pushed the bugfix-snapshotstatus-tostring branch from 0c41110 to 2b7de1a Compare October 5, 2017 09:10
@liketic
Copy link
Author

liketic commented Oct 5, 2017

@cbuescher Rebase done.

@cbuescher
Copy link
Member

@elasticmachine lets test this again, shall we?

@cbuescher cbuescher merged commit a978ddf into elastic:master Oct 5, 2017
cbuescher pushed a commit that referenced this pull request Oct 5, 2017
@cbuescher
Fix toString() in SnapshotStatus (#26852)
2e4d69a 
Closes #26851
@cbuescher
Copy link
Member

@liketic finally a good test run, I merged this to master and 6.1. Thanks for this PR.

@liketic
Copy link
Author

liketic commented Oct 5, 2017

@cbuescher finally, we got it... Thanks for your patient.

@liketic liketic deleted the bugfix-snapshotstatus-tostring branch October 5, 2017 11:06
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Oct 6, 2017
@jasontedor
Merge branch 'ccr' into engine-factory-provider
5506cfd 
* ccr: (42 commits)
  [DOCS] Added info about snapshotting your data before an upgrade.
  Add documentation about disabling `_field_names`. (elastic#26813)
  Remove UnsortedNumericDoubleValues (elastic#26817)
  Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856)
  [TEST] Added skipping the `headers` feature to the Bulk REST YAML test
  Update type-field.asciidoc
  Fix search_after with geo distance sorting (elastic#26891)
  Use proper logging placeholder for Netty logging
  Add Netty channel information on write and flush failure
  Remove deploying in JBoss documentation
  Document JVM option MaxFDLimit for macOS ()
  Add additional low-level logging handler ()
  Unwrap causes when maybe dying
  Change log level on write and flush failure to warn
  [TEST] add test to ensure legacy list syntax in yml works fine
  Bump BWC version for settings serialization to 6.1.0
  Removed void token filter entries and added two tests
  Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238)
  Fix toString() in SnapshotStatus (elastic#26852)
  elastic#26870 change bwc version for fuzzy_transpositions to 6.1 after backport
  ...
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Oct 6, 2017
@jasontedor
Merge branch 'ccr' into newSnapshotBetweenMinAndMaxSeqNo
2e27e0b 
* ccr: (110 commits)
  Use LF line endings in Painless generated files (elastic#26822)
  [DOCS] Added info about snapshotting your data before an upgrade.
  Add documentation about disabling `_field_names`. (elastic#26813)
  Remove UnsortedNumericDoubleValues (elastic#26817)
  Fix IndexOutOfBoundsException in histograms for NaN doubles (elastic#26787) (elastic#26856)
  [TEST] Added skipping the `headers` feature to the Bulk REST YAML test
  Update type-field.asciidoc
  Fix search_after with geo distance sorting (elastic#26891)
  Use proper logging placeholder for Netty logging
  Add Netty channel information on write and flush failure
  Remove deploying in JBoss documentation
  Document JVM option MaxFDLimit for macOS ()
  Add additional low-level logging handler ()
  Unwrap causes when maybe dying
  Change log level on write and flush failure to warn
  [TEST] add test to ensure legacy list syntax in yml works fine
  Bump BWC version for settings serialization to 6.1.0
  Removed void token filter entries and added two tests
  Added Bengali Analyzer to Elasticsearch with respect to the lucene update(PR#238)
  Fix toString() in SnapshotStatus (elastic#26852)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbuescher cbuescher cbuescher approved these changes

Assignees

@cbuescher cbuescher

Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >enhancement v6.1.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liketic @elasticmachine @cbuescher @colings86