7227 : Prohibit Non Final Static Field #8558

SandishKumarHN · 2019-09-19T00:34:51Z

Fixes #7227
Prohibit Non-Final Static Field
Add annotation if required static fields @SuppressWarnings("SSBasedInspection")

This PR has:

been self-reviewed.
- using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
added integration tests.
been tested in a test Druid cluster.

Key changed/added classes in this PR

MyFoo
OurBar
TheirBaz

ccaominh · 2019-09-19T17:16:53Z

Please update the PR description (e.g., first line should be "Fixes #7227.", remove placeholder text, etc.).

SandishKumarHN · 2019-09-19T18:11:46Z

@ccaominh Thanks for quick comments changed, please review now

…ibit-StaticNon-FinalFields

SandishKumarHN · 2019-09-20T23:09:22Z

@leventov can you please review this also.

leventov · 2019-09-21T10:50:16Z

benchmarks/src/main/java/org/apache/druid/benchmark/FloatCompressionBenchmark.java

@@ -55,12 +55,15 @@
 @OutputTimeUnit(TimeUnit.MILLISECONDS)
 public class FloatCompressionBenchmark
 {
+  @SuppressWarnings("SSBasedInspection")


Please try to minimize these suppressions. In most *Benchmark and *Test classes static fields could be converted into instance fields.

In classes where this cannot be done, it makes sense to add @SuppressWarnings("SSBasedInspection") once on the class declaration instead of separately on every field.

In any case, each @SuppressWarnings("SSBasedInspection") should have a comment after it specifying the concrete type of Structural Search inspection being suppressed. In this PR, many of them should all look like

@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block

leventov · 2019-09-21T10:51:26Z

benchmarks/src/main/java/org/apache/druid/benchmark/FilterPartitionBenchmark.java

@@ -124,8 +124,8 @@

  private BenchmarkSchemaInfo schemaInfo;

-  private static String JS_FN = "function(str) { return 'super-' + str; }";
-  private static ExtractionFn JS_EXTRACTION_FN = new JavaScriptExtractionFn(JS_FN, false, JavaScriptConfig.getEnabledInstance());
+  private static final String JS_FN = "function(str) { return 'super-' + str; }";


Static fields should be ordered before instance fields.

SandishKumarHN · 2019-09-22T03:30:41Z

@leventov thanks for the review, made changes based on your suggestions.

leventov · 2019-09-22T12:05:58Z

benchmarks/src/main/java/org/apache/druid/benchmark/FloatCompressionBenchmarkFileGenerator.java

@@ -42,6 +42,7 @@
 import java.util.List;
 import java.util.Map;

+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


The comment is not true: field dirPath is not set in an initializer block.

SandishKumarHN · 2019-09-25T05:49:15Z

@leventov made changes

SandishKumarHN · 2019-09-25T21:54:21Z

@leventov "Inspections: pull requests (Druid) — TeamCity build failed" is not related to PR.

leventov · 2019-09-26T15:53:46Z

benchmarks/src/main/java/org/apache/druid/benchmark/FloatCompressionBenchmarkFileGenerator.java

@@ -42,6 +42,7 @@
 import java.util.List;
 import java.util.Map;

+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an static method


If the suppression is done for a single field (dirPath), it should be on the field still. My suggestion to move the suppressions up only applied when there are more than one field in the class which needs to be suppressed.

leventov · 2019-09-26T15:54:06Z

benchmarks/src/main/java/org/apache/druid/benchmark/IncrementalIndexRowTypeBenchmark.java

@@ -51,7 +51,7 @@
  private IncrementalIndex incIndex;
  private IncrementalIndex incFloatIndex;
  private IncrementalIndex incStrIndex;
-  private static AggregatorFactory[] aggs;
+  private static final AggregatorFactory[] AGGS;


Please move static fields above instance fields.

leventov · 2019-09-26T15:54:44Z

benchmarks/src/main/java/org/apache/druid/benchmark/LongCompressionBenchmarkFileGenerator.java

@@ -42,6 +42,7 @@
 import java.util.List;
 import java.util.Map;

+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an static method


If the suppression is done for a single field (dirPath), it should be on the field still.

leventov · 2019-09-26T15:56:24Z

benchmarks/src/main/java/org/apache/druid/benchmark/query/ScanBenchmark.java

@@ -100,6 +100,7 @@
 @Fork(value = 1)
 @Warmup(iterations = 10)
 @Measurement(iterations = 25)
+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because variable is not initilized.


If this is about ordering field, I think it could be an instance field. The order of static and instance field is also disturbed.

leventov · 2019-09-26T15:57:26Z

core/src/main/java/org/apache/druid/common/config/NullHandling.java

@@ -29,6 +29,7 @@
 * introduced as part of https://github.com/apache/incubator-druid/issues/4349 and the old druid behavior
 * where null values are replaced with default values e.g Null Strings are replaced with empty values.
 */
+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an static method


If the suppression is done for a single field (INSTANCE), it should be on the field still.

SandishKumarHN · 2019-09-27T05:34:25Z

@leventov fixed

SandishKumarHN · 2019-10-02T01:05:23Z

@leventov looks like a code inspection issue, which is not related to this PR. verified the code I don't see any duplicate code.

leventov · 2019-10-14T14:42:49Z

There are violations in SimpleTestIndex.java

SandishKumarHN · 2019-10-15T23:50:42Z

@leventov inspection failure from
SeekableStreamSupervisor.java (4) 175: TaskGroup() Optional<DateTime> used as type for parameter 'minimumMessageTime'

leventov · 2019-10-16T07:26:01Z

There are 7 related errors:

https://teamcity.jetbrains.com/viewLog.html?buildId=2590045&tab=Inspection&buildTypeId=OpenSourceProjects_Druid_InspectionsPullRequests

leventov · 2019-10-16T07:27:11Z

@SandishKumarHN can you see these errors yourself here, or there is some sort of access problem that prevents your (a non-admin user) from seeing these problems?

leventov

Reviewed until SchemalessIndexTest

leventov · 2019-10-16T16:30:05Z

benchmarks/src/main/java/org/apache/druid/benchmark/query/ScanBenchmark.java

@@ -102,6 +102,10 @@
 @Measurement(iterations = 25)
 public class ScanBenchmark
 {
+  @Param({"NONE", "DESCENDING", "ASCENDING"})
+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block
+  private static ScanQuery.Order ordering;


This field is not set in an initializer block. Please check the truthfulness of the comments that you add.

Since this is @Param, it shouldn't be static at all. Could you please prohibit static @Params by adding another structured search pattern:

@Param($Values$) static $T$ $param$;

in this PR?

leventov · 2019-10-16T16:32:10Z

core/src/main/java/org/apache/druid/common/config/NullHandling.java

@@ -50,6 +50,7 @@
   * It does not take effect in all unit tests since we don't use Guice Injection.
   */
  @Inject
+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


Again, has nothing to do with an initializer block. In this case, it's because this field is injected it cannot be final.

leventov · 2019-10-16T16:33:02Z

core/src/main/java/org/apache/druid/java/util/common/io/NativeIO.java

@@ -35,9 +35,10 @@
 /**
 * Native I/O operations in order to minimize cache impact.
 */
+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


Please move this suppression to FIELD.

leventov · 2019-10-16T16:34:33Z

core/src/main/java/org/apache/druid/java/util/emitter/EmittingLogger.java

@@ -40,6 +40,7 @@
  public static final String EXCEPTION_MESSAGE_KEY = "exceptionMessage";
  public static final String EXCEPTION_STACK_TRACE_KEY = "exceptionStackTrace";

+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


Same problem here, the comment doesn't reflect the reality. registerEmitter() is the reason.

leventov · 2019-10-16T16:35:05Z

core/src/main/java/org/apache/druid/utils/JvmUtils.java

@@ -50,11 +50,12 @@ public static boolean isIsJava9Compatible()
  }

  @Inject
-  private static RuntimeInfo runtimeInfo = new RuntimeInfo();
+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


leventov · 2019-10-16T17:08:23Z

processing/src/test/java/org/apache/druid/query/extraction/MapLookupExtractionFnSerDeTest.java

@@ -39,6 +39,7 @@
 */
 public class MapLookupExtractionFnSerDeTest
 {
+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


leventov · 2019-10-16T17:08:39Z

processing/src/test/java/org/apache/druid/query/filter/InDimFilterSerDesrTest.java

@@ -35,6 +35,8 @@

 public class InDimFilterSerDesrTest
 {
+
+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block


leventov · 2019-10-16T17:08:58Z

processing/src/test/java/org/apache/druid/query/filter/IntervalDimFilterTest.java

@@ -37,6 +37,7 @@

 public class IntervalDimFilterTest
 {
+  @SuppressWarnings("SSBasedInspection")  // static field(s) cannot be final because set in an initializer block


leventov · 2019-10-16T17:11:42Z

...essing/src/test/java/org/apache/druid/query/scan/ScanResultValueTimestampComparatorTest.java

@@ -36,6 +36,7 @@

 public class ScanResultValueTimestampComparatorTest
 {
+  @SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an initializer block
  private static QuerySegmentSpec intervalSpec;


setup() can be inlined

leventov · 2019-10-16T17:16:47Z

processing/src/test/java/org/apache/druid/segment/SchemalessIndexTest.java

@@ -64,9 +64,10 @@

 /**
 */
+@SuppressWarnings("SSBasedInspection") // static field(s) cannot be final because set in an static method


mergedIndex doesn't need to be static. It seems to be that index doesn't need to be static, too. getIncrementalIndex() and makeIncrementalIndex() should become non-static, too.

stale · 2019-12-15T18:17:21Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2020-01-12T18:43:46Z

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

Prohibit Non Final Static Field

654037f

Merge remote-tracking branch 'remotes/upstream/master' into 7227-Proh…

209f360

…ibit-StaticNon-FinalFields

leventov reviewed Sep 21, 2019

View reviewed changes

review changes-1

a1e894e

leventov requested changes Sep 22, 2019

View reviewed changes

SandishKumarHN added 2 commits September 23, 2019 21:36

review changes-3

b5457b2

review changes-3

2c8a72e

leventov reviewed Sep 26, 2019

View reviewed changes

review changes-4

06b5f70

SandishKumarHN force-pushed the 7227-Prohibit-StaticNon-FinalFields branch from 2057858 to 06b5f70 Compare September 27, 2019 00:48

merging with master

72ac63e

SandishKumarHN force-pushed the 7227-Prohibit-StaticNon-FinalFields branch from ecce209 to 72ac63e Compare October 1, 2019 20:28

review changes

bee8817

SandishKumarHN requested a review from leventov October 14, 2019 17:48

leventov requested changes Oct 16, 2019

View reviewed changes

stale bot added the stale label Dec 15, 2019

stale bot closed this Jan 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7227 : Prohibit Non Final Static Field #8558

7227 : Prohibit Non Final Static Field #8558

SandishKumarHN commented Sep 19, 2019 •

edited

Loading

ccaominh commented Sep 19, 2019

SandishKumarHN commented Sep 19, 2019

SandishKumarHN commented Sep 20, 2019

leventov Sep 21, 2019

leventov Sep 21, 2019

SandishKumarHN commented Sep 22, 2019 •

edited

Loading

leventov Sep 22, 2019

SandishKumarHN commented Sep 25, 2019

SandishKumarHN commented Sep 25, 2019

leventov Sep 26, 2019

leventov Sep 26, 2019

leventov Sep 26, 2019

leventov Sep 26, 2019

leventov Sep 26, 2019

SandishKumarHN commented Sep 27, 2019

SandishKumarHN commented Oct 2, 2019

leventov commented Oct 14, 2019

SandishKumarHN commented Oct 15, 2019

leventov commented Oct 16, 2019

leventov commented Oct 16, 2019

leventov left a comment

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

leventov Oct 16, 2019

stale bot commented Dec 15, 2019

stale bot commented Jan 12, 2020

7227 : Prohibit Non Final Static Field #8558

7227 : Prohibit Non Final Static Field #8558

Conversation

SandishKumarHN commented Sep 19, 2019 • edited Loading

Key changed/added classes in this PR

ccaominh commented Sep 19, 2019

SandishKumarHN commented Sep 19, 2019

SandishKumarHN commented Sep 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandishKumarHN commented Sep 22, 2019 • edited Loading

Choose a reason for hiding this comment

SandishKumarHN commented Sep 25, 2019

SandishKumarHN commented Sep 25, 2019

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

SandishKumarHN commented Sep 27, 2019

SandishKumarHN commented Oct 2, 2019

leventov commented Oct 14, 2019

SandishKumarHN commented Oct 15, 2019

leventov commented Oct 16, 2019

leventov commented Oct 16, 2019

leventov 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

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

stale bot commented Dec 15, 2019

stale bot commented Jan 12, 2020

SandishKumarHN commented Sep 19, 2019 •

edited

Loading

SandishKumarHN commented Sep 22, 2019 •

edited

Loading