Get rid of "unused variable" warnings #31876

cbuescher · 2018-07-06T19:32:43Z

Another clean up trying to get rid of some annoying warnings. One large group that is
reasonably easy to tackle is the "unused variable" warnings. There are several cases
were most likely want to suppress the warnings (especially in the client documentation test
where the snippets should contain unused variable names). Most of the things can just be deleted though.

This is most likey a leftover from 24d10ad and can go.

elasticmachine · 2018-07-09T10:19:35Z

Pinging @elastic/es-core-infra

cbuescher · 2018-07-09T10:08:20Z

modules/lang-painless/src/main/java/org/elasticsearch/painless/node/SSource.java

@@ -130,7 +130,8 @@ public int getMaxLoopCounter() {
    private final ScriptClassInfo scriptClassInfo;
    private final CompilerSettings settings;
    private final String name;
-    private final String source;
+    // TODO can this be removed all up the call chain?
+    // private final String source;


@jdconrad I couldn't find any reference to this field, but maybe its here for debugging purposes or anything the like? In that case I would add a comment instead of removing. If this is really unused, maybe it can also be removed from the method signatures passing it down here. Could you take a look and comment?

Will take a look! Thanks for pointing this out.

@cbuescher Looks like this is now extraneous. It never got removed once source was set in the class above. Please feel free to remove this as part of this PR or if you'd prefer I can remove it in a different PR.

will do, thanks

cbuescher · 2018-07-09T10:33:34Z

.../test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java

-        Request replicaRequest = (Request) primary.perform(request).replicaRequest;
+        // TODO check if the following two lines are really unused or might have side effects
+        // final Request request = new Request();
+        // Request replicaRequest = (Request) primary.perform(request).replicaRequest;


@ywelsch replicaRequest was used to test the primaryTerm() with "assertThat(replicaRequest.primaryTerm(), equalTo(primaryTerm));". Since that got removed, this part of the test can also go I guess. Does the test still perform its function after this? After all the primaryTerm gets mocked out somewhere above, but never asserted anywhere later? Could you take a look and comment?

you can delete this test. It doesn't do anything meaningful.

...est-high-level/src/test/java/org/elasticsearch/client/documentation/CRUDDocumentationIT.java

Currently Role.Builder keeps a reference to the FieldPermissionsCache that is passed into its constructors. This seems to be unused except for passing it on to convertFromIndicesPrivileges() in the second ctor itself, but we don't need to keep the internal reference in that case, so it can be removed. Relates to elastic#31876

Currently Role.Builder keeps a reference to the FieldPermissionsCache that is passed into its constructors. This seems to be unused except for passing it on to convertFromIndicesPrivileges() in the second ctor itself, but we don't need to keep the internal reference in that case, so it can be removed. Relates to #31876

cbuescher · 2018-07-11T08:09:19Z

All the open questions are adressed now, I think this is ready for a full review now. Spin off in #31935 can be merged separately but is also included in this PR.

Conflicts: client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SnapshotClientDocumentationIT.java server/src/test/java/org/elasticsearch/index/query/GeoDistanceQueryBuilderTests.java x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java

danielmitterdorfer

Thanks for updating the PR. I left a couple of comments.

libs/grok/src/main/java/org/elasticsearch/grok/Grok.java

libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java

...s/analysis-common/src/main/java/org/elasticsearch/analysis/common/KeepWordFilterFactory.java

danielmitterdorfer · 2018-09-25T10:32:22Z

modules/parent-join/src/test/java/org/elasticsearch/join/aggregations/ChildrenIT.java

        final Set<String> articleIds = new HashSet<>();
        final Set<String> commentIds = new HashSet<>();
        final Map<String, Set<String>> commenterToCommentId = new HashMap<>();

        private Control(String category) {
-            this.category = category;
+            // category not used any further


Does it make sense to remove the constructor argument then as well?

Yes, makes sense to remove the whole constructor actually since the class is private already and only used inside this test class.

danielmitterdorfer · 2018-09-25T10:41:04Z

server/src/main/java/org/elasticsearch/common/lucene/search/XMoreLikeThis.java

        }

-        void update(String word, String topField, float score, float idf, int docFreq, int tf) {
+        public void update(String word, String topField, float score) {


Is it necessary to increase the visibility of this method?

No, not sure how this happend. Removing.

test/framework/src/main/java/org/elasticsearch/test/store/MockFSDirectoryService.java

x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/LogicalPlanBuilder.java

danielmitterdorfer · 2018-09-25T11:20:49Z

...gin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/SourceGeneratorTests.java

@@ -64,6 +64,7 @@ public void testLimit() {
        SearchSourceBuilder sourceBuilder = SourceGenerator.sourceBuilder(container, null, randomIntBetween(1, 10));
        Builder aggBuilder = sourceBuilder.aggregations();
        assertEquals(1, aggBuilder.count());
+        @SuppressWarnings("unused")


I wonder whether we should provide an accessor instead and uncomment the line below?

Good point.

...r/src/main/java/org/elasticsearch/xpack/watcher/notification/pagerduty/PagerDutyAccount.java

...r/src/test/java/org/elasticsearch/xpack/watcher/test/AbstractWatcherIntegrationTestCase.java

cbuescher · 2018-09-25T12:54:11Z

@danielmitterdorfer thanks a lot for getting back to this. I addressed most of your comments with the latest commit, checking back on how MovAvgIT is supposed to work. Could you go through the other comments I replied to and see if you agree?

danielmitterdorfer · 2018-09-25T13:48:16Z

server/src/test/java/org/elasticsearch/index/query/GeoDistanceQueryBuilderTests.java

        parseQuery(query).toQuery(createShardContext());
-     // TODO: what can we check?
+        // TODO: what can we check? See https://github.com/elastic/elasticsearch/issues/34043


Thanks for creating the issue!

danielmitterdorfer

Thanks @cbuescher! LGTM

s1monw · 2018-09-26T21:46:02Z

@cbuescher I think this change caused some failures I am hitting locally. It reproduces on master:

./gradlew :server:integTest -Dtests.seed=CE15410BFF4D8B8A -Dtests.class=org.elasticsearch.search.aggregations.pipeline.moving.avg.MovAvgIT -Dtests.method="testHoltWintersNotEnoughData" -Dtests.security.manager=true -Dtests.locale=fi -Dtests.timezone=JST -Dcompiler.java=10 -Druntime.java=8

2> REPRODUCE WITH: ./gradlew :server:integTest -Dtests.seed=CE15410BFF4D8B8A -Dtests.class=org.elasticsearch.search.aggregations.pipeline.moving.avg.MovAvgIT -Dtests.method="testHoltWintersNotEnoughData" -Dtests.security.manager=true -Dtests.locale=fi -Dtests.timezone=JST -Dcompiler.java=10 -Druntime.java=8
FAILURE 0.39s | MovAvgIT.testHoltWintersNotEnoughData <<< FAILURES!
   > Throwable #1: junit.framework.AssertionFailedError: Expected exception SearchPhaseExecutionException but no exception was thrown
   >    at __randomizedtesting.SeedInfo.seed([CE15410BFF4D8B8A:A5C013FEF4A98B75]:0)
   >    at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2687)
   >    at org.apache.lucene.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2672)
   >    at org.elasticsearch.search.aggregations.pipeline.moving.avg.MovAvgIT.testHoltWintersNotEnoughData(MovAvgIT.java:856)
   >    at java.lang.Thread.run(Thread.java:748)

s1monw · 2018-09-26T21:49:54Z

I opened #34098

This change cleans up "unused variable" warnings. There are several cases were we most likely want to suppress the warnings (especially in the client documentation test where the snippets contain many unused variables). In a lot of cases the unused variables can just be deleted though.

WIP remove unused variable warnings

3fd9cf6

cbuescher added >non-issue WIP labels Jul 6, 2018

cbuescher self-assigned this Jul 9, 2018

Remove TODO

6ff4802

This is most likey a leftover from 24d10ad and can go.

colings86 added the :Core/Infra/Core Core issues without another label label Jul 9, 2018

iter

efc0a46

cbuescher commented Jul 9, 2018

View reviewed changes

rjernst reviewed Jul 10, 2018

View reviewed changes

...est-high-level/src/test/java/org/elasticsearch/client/documentation/CRUDDocumentationIT.java Show resolved Hide resolved

Christoph Büscher added 3 commits July 10, 2018 12:50

Remove version variables, leftovers from 6861d35

ae0b69d

Merge branch 'master' into remove-unused-variables

ebd5f95

ScoreMode isn't used anymore, its CombineFunctions now

076c36c

cbuescher mentioned this pull request Jul 10, 2018

Remove unused reference to filePermissionsCache #31923

Merged

Christoph Büscher added 5 commits July 10, 2018 15:19

iter

a2ec6cd

iter

d7b5e41

Remove loop that is never going to be executed

77820c7

Remove unused params from SSource and Walker

ca69bdc

Merge branch 'master' into remove-unused-variables

793fbe0

cbuescher mentioned this pull request Jul 10, 2018

Remove unused params from SSource and Walker #31935

Merged

Merge branch 'master' into remove-unused-variables

4aa37f2

cbuescher added review v7.0.0 and removed WIP labels Jul 11, 2018

cbuescher changed the title ~~WIP Get rid of "unused variable" warnings~~ Get rid of "unused variable" warnings Jul 11, 2018

Merge branch 'master' into remove-unused-variables

b276197

Re-add accidentally removed import

2720ae3

cbuescher removed their assignment Jul 20, 2018

Christoph Büscher added 8 commits July 20, 2018 12:17

Merge branch 'master' into remove-unused-variables

4bb6045

Merge branch 'master' into remove-unused-variables

fb96b0c

Merge branch 'master' into remove-unused-variables

643f2b5

Merge branch 'master' into remove-unused-variables

95f1adf

Merge branch 'master' into remove-unused-variables

fd344db

Merge branch 'master' into remove-unused-variables

95d8624

Merge branch 'master' into remove-unused-variables

ff0d7f4

cbuescher force-pushed the remove-unused-variables branch from d42f896 to ff0d7f4 Compare September 20, 2018 13:42

Merge branch 'master' into remove-unused-variables

52979f1

danielmitterdorfer reviewed Sep 25, 2018

View reviewed changes

iteration: adressing comments

654f350

cbuescher mentioned this pull request Sep 25, 2018

[Test] MovAvgIT#testBadModelParams needs clarification #34046

Closed

Muting MovAvgIT#testBadModelParams

f407932

danielmitterdorfer reviewed Sep 25, 2018

View reviewed changes

iter

21118b1

danielmitterdorfer approved these changes Sep 25, 2018

View reviewed changes

Christoph Büscher added 2 commits September 25, 2018 18:20

Revert removing code in TemplateUpdateServiceTests

460a7fa

Merge branch 'master' into remove-unused-variables

a5cc0c8

cbuescher merged commit ba3ceea into elastic:master Sep 26, 2018

s1monw mentioned this pull request Sep 26, 2018

[CI] MovAvgIT.testHoltWintersNotEnoughData #34098

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get rid of "unused variable" warnings #31876

Get rid of "unused variable" warnings #31876

cbuescher commented Jul 6, 2018 •

edited

Loading

elasticmachine commented Jul 9, 2018

cbuescher Jul 9, 2018

jdconrad Jul 10, 2018

jdconrad Jul 10, 2018

cbuescher Jul 10, 2018

cbuescher Jul 9, 2018

ywelsch Jul 9, 2018

cbuescher commented Jul 11, 2018

danielmitterdorfer left a comment

danielmitterdorfer Sep 25, 2018

cbuescher Sep 25, 2018

danielmitterdorfer Sep 25, 2018

cbuescher Sep 25, 2018

danielmitterdorfer Sep 25, 2018

cbuescher Sep 25, 2018

cbuescher commented Sep 25, 2018

danielmitterdorfer Sep 25, 2018

danielmitterdorfer left a comment

s1monw commented Sep 26, 2018

s1monw commented Sep 26, 2018

Get rid of "unused variable" warnings #31876

Get rid of "unused variable" warnings #31876

Conversation

cbuescher commented Jul 6, 2018 • edited Loading

elasticmachine commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Jul 11, 2018

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbuescher commented Sep 25, 2018

Choose a reason for hiding this comment

danielmitterdorfer left a comment

Choose a reason for hiding this comment

s1monw commented Sep 26, 2018

s1monw commented Sep 26, 2018

cbuescher commented Jul 6, 2018 •

edited

Loading