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 line length offenders in the o.e.search package. #36223

Merged
merged 5 commits into from
Dec 6, 2018
Merged

Fix line length offenders in the o.e.search package. #36223

merged 5 commits into from
Dec 6, 2018

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Dec 4, 2018

Relates #34884

@dakrone dakrone mentioned this pull request Dec 4, 2018
Closed
33 tasks
@jpountz jpountz added the :Core/Infra/Core Core issues without another label label Dec 4, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

nik9000
nik9000 reviewed Dec 4, 2018
View reviewed changes
server/src/main/java/org/elasticsearch/search/aggregations/metrics/HyperLogLogPlusPlus.java
297.1182, 299.4012, 300.6352, 302.1354, 304.1756, 306.1606, 307.3462, 308.5214, 309.4134, 310.8352, 313.9684, 315.837,
316.7796, 318.9858, },
// precision 7
{ 92, 93.4934, 94.9758, 96.4574, 97.9718, 99.4954, 101.5302, 103.0756, 104.6374, 106.1782, 107.7888, 109.9522, 111.592,
Copy link
Member

Choose a reason for hiding this comment

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

hahahahahhaahahha

server/src/main/java/org/elasticsearch/search/aggregations/metrics/HyperLogLogPlusPlus.java Outdated
return Objects.equals(p, other.p) &&
Objects.equals(algorithm.get(bucket), other.algorithm.get(bucket)) &&
Objects.equals(getComparableData(bucket), other.getComparableData(bucket));
return Objects.equals(p, other.p) && Objects.equals(algorithm.get(bucket), other.algorithm.get(bucket))
Copy link
Member

Choose a reason for hiding this comment

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

Could you do these one per line?

server/src/test/java/org/elasticsearch/search/aggregations/bucket/ShardSizeTestCase.java Outdated
@@ -90,7 +90,8 @@ protected void indexData() throws Exception {

indexRandom(true, docs);

SearchResponse resp = client().prepareSearch("idx").setTypes("type").setRouting(routing1).setQuery(matchAllQuery()).execute().actionGet();
SearchResponse resp = client().prepareSearch("idx").setTypes("type").setRouting(routing1).setQuery(matchAllQuery()).execute()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just use get?

server/src/test/java/org/elasticsearch/search/basic/SearchWithRandomIOExceptionsIT.java Outdated
final int numSearches = scaledRandomIntBetween(10, 20);
// we don't check anything here really just making sure we don't leave any open files or a broken index behind.
for (int i = 0; i < numSearches; i++) {
try {
int docToQuery = between(0, numDocs - 1);
int expectedResults = added[docToQuery] ? 1 : 0;
logger.info("Searching for [test:{}]", English.intToEnglish(docToQuery));
SearchResponse searchResponse = client().prepareSearch().setTypes("type").setQuery(QueryBuilders.matchQuery("test", English.intToEnglish(docToQuery)))
SearchResponse searchResponse = client().prepareSearch().setTypes("type")
.setQuery(QueryBuilders.matchQuery("test", English.intToEnglish(docToQuery)))
Copy link
Member

Choose a reason for hiding this comment

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

I think this indentation doesn't line up with the line under it which'll be confusing to read.

server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxIT.java Outdated
@@ -225,7 +229,8 @@ public void testCompleteLonRange() throws Exception {
assertThat(searchResponse.getHits().getTotalHits(), equalTo(1L));
searchResponse = client().prepareSearch()
.setQuery(
geoBoundingBoxQuery("location").setValidationMethod(GeoValidationMethod.COERCE).setCorners(50, 0, -50, 360).type("indexed")
geoBoundingBoxQuery("location").setValidationMethod(GeoValidationMethod.COERCE).setCorners(50, 0, -50, 360)
.type("indexed")
Copy link
Member

Choose a reason for hiding this comment

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

Having this line on the same indention of as the one that builds the object that it is poking doesn't quite feel right to me.

server/src/test/java/org/elasticsearch/search/scroll/SearchScrollIT.java Outdated
client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();

client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForGreenStatus().execute().actionGet();

for (int i = 0; i < 100; i++) {
client().prepareIndex("test", "type1", Integer.toString(i)).setSource(jsonBuilder().startObject().field("field", i).endObject()).execute().actionGet();
client().prepareIndex("test", "type1", Integer.toString(i)).setSource(jsonBuilder().startObject().field("field", i).endObject())
.execute().actionGet();
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't look right.

server/src/test/java/org/elasticsearch/search/suggest/CompletionSuggestSearchIT.java Outdated
@@ -1027,7 +1053,8 @@ public void testPrunedSegments() throws IOException {
.field("somefield", "somevalue")
.endObject()
).get(); // we have 2 docs in a segment...
ForceMergeResponse actionGet = client().admin().indices().prepareForceMerge().setFlush(true).setMaxNumSegments(1).execute().actionGet();
ForceMergeResponse actionGet = client().admin().indices().prepareForceMerge().setFlush(true).setMaxNumSegments(1).execute()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just call get?

@jpountz jpountz requested a review from nik9000 December 5, 2018 09:57
nik9000
nik9000 approved these changes Dec 5, 2018
View reviewed changes
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.

LGTM. Thanks for all that cleaning!

@nik9000
Copy link
Member

nik9000 commented Dec 5, 2018

I'm sorry for your merge conflicts!

@jpountz
Copy link
Contributor Author

jpountz commented Dec 6, 2018

No worries, that shouldn't be too hard to fix. Thanks for reviewing!

@jpountz jpountz merged commit b08cffa into elastic:master Dec 6, 2018
@jpountz jpountz deleted the fix/line_length_search branch December 6, 2018 15:31
jpountz added a commit that referenced this pull request Dec 6, 2018
@jpountz
Fix line length offenders in the o.e.search package. (#36223)
5f3893f 
Relates #34884
jpountz added a commit that referenced this pull request Dec 6, 2018
@jpountz
Revert "Fix line length offenders in the o.e.search package. (#36223)"
a67c91e 
This reverts commit 5f3893f.
@jpountz
Copy link
Contributor Author

jpountz commented Dec 7, 2018

I am not backporting this one to 6.x as there is a number of conflicts that make this change unsafe. An alternative would be to re-do the change on 6.x but it is very time-consuming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
:Core/Infra/Core Core issues without another label >non-issue v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jpountz @elasticmachine @nik9000 @colings86