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

Fix compiler warnings in :server - part 3 #76024

Merged
3 changes: 3 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ ij_continuation_indent_size = 2
indent_size = 2
max_line_length = 150

[*.md]
max_line_length = 80

[*.groovy]
indent_size = 4
ij_continuation_indent_size = 4
Expand Down
83 changes: 46 additions & 37 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Contributing to elasticsearch
=============================

Elasticsearch is a free and open project and we love to receive contributions from our community — you! There are many ways to contribute, from writing tutorials or blog posts, improving the documentation, submitting bug reports and feature requests or writing code which can be incorporated into Elasticsearch itself.
Elasticsearch is a free and open project and we love to receive contributions from our community — you! There are many ways to contribute, from writing tutorials or blog posts, improving the documentation, submitting bug reports and feature requests or writing code which can be incorporated into Elasticsearch itself.

If you want to be rewarded for your contributions, sign up for the [Elastic Contributor Program](https://www.elastic.co/community/contributor). Each time you
make a valid contribution, you’ll earn points that increase your chances of winning prizes and being recognized as a top contributor.
Expand Down Expand Up @@ -38,14 +38,14 @@ Contributing code and documentation changes
-------------------------------------------

If you would like to contribute a new feature or a bug fix to Elasticsearch,
please discuss your idea first on the Github issue. If there is no Github issue
please discuss your idea first on the GitHub issue. If there is no GitHub issue
for your idea, please open one. It may be that somebody is already working on
it, or that there are particular complexities that you should know about before
starting the implementation. There are often a number of ways to fix a problem
and it is important to find the right approach before spending time on a PR
that cannot be merged.

We add the `help wanted` label to existing Github issues for which community
We add the `help wanted` label to existing GitHub issues for which community
contributions are particularly welcome, and we use the `good first issue` label
to mark issues that we think will be suitable for new contributors.

Expand Down Expand Up @@ -147,7 +147,6 @@ and then run `curl` in another window like this:
curl -u elastic:password localhost:9200



### Importing the project into IntelliJ IDEA

The minimum IntelliJ IDEA version required to import the Elasticsearch project is 2020.1
Expand Down Expand Up @@ -427,26 +426,25 @@ We require license headers on all Java files. With the exception of the
top-level `x-pack` directory, all contributed code should have the following
license header unless instructed otherwise:

/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

The top-level `x-pack` directory contains code covered by the [Elastic
license](licenses/ELASTIC-LICENSE-2.0.txt). Community contributions to this code are
welcome, and should have the following license header unless instructed
otherwise:

/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

It is important that the only code covered by the Elastic licence is contained
within the top-level `x-pack` directory. The build will fail its pre-commit
Expand All @@ -456,52 +454,63 @@ checks if contributed code does not have the appropriate license headers.
> be automatically configured to add the correct license header to new source
> files based on the source location.

### Type-checking, generics and casting

You should try to write code that does not require suppressing any warnings from
the compiler, e.g. suppressing type-checking, raw generics, and so on. However,
this isn't always possible or practical. In such cases, you should use the
`@SuppressWarnings` annotations to silence the compiler warning, trying to keep
the scope of the suppression as small as possible. Where a piece of code
requires a lot of suppressions, it may be better to apply a single suppression
at a higher level e.g. at the method or even class level. Use your judgement.

There are also cases where the compiler simply refuses to accept an assignment
or cast of any kind, because it lacks the information to know that the types are
OK. In such cases, you can use
the [`Types.forciblyCast`](libs/core/src/main/java/org/elasticsearch/core/Types.java)
utility method. As the name suggests, you can coerce any type to any other type,
so please use it as a last resort.

### Creating A Distribution

Run all build commands from within the root directory:

```sh
cd elasticsearch/
```
cd elasticsearch/

To build a darwin-tar distribution, run this command:

```sh
./gradlew -p distribution/archives/darwin-tar assemble
```
./gradlew -p distribution/archives/darwin-tar assemble

You will find the distribution under:
`./distribution/archives/darwin-tar/build/distributions/`

./distribution/archives/darwin-tar/build/distributions/

To create all build artifacts (e.g., plugins and Javadocs) as well as
distributions in all formats, run this command:

```sh
./gradlew assemble
```
./gradlew assemble

> **NOTE:** Running the task above will fail if you don't have a available
> **NOTE:** Running the task above will fail if you don't have an available
> Docker installation.

The package distributions (Debian and RPM) can be found under:
`./distribution/packages/(deb|rpm|oss-deb|oss-rpm)/build/distributions/`

./distribution/packages/(deb|rpm|oss-deb|oss-rpm)/build/distributions/

The archive distributions (tar and zip) can be found under:
`./distribution/archives/(darwin-tar|linux-tar|windows-zip|oss-darwin-tar|oss-linux-tar|oss-windows-zip)/build/distributions/`

./distribution/archives/(darwin-tar|linux-tar|windows-zip|oss-darwin-tar|oss-linux-tar|oss-windows-zip)/build/distributions/

### Running The Full Test Suite

Before submitting your changes, run the test suite to make sure that nothing is broken, with:

```sh
./gradlew check
```
./gradlew check

If your changes affect only the documentation, run:

```sh
./gradlew -p docs check
```
./gradlew -p docs check

For more information about testing code examples in the documentation, see
https://github.com/elastic/elasticsearch/blob/master/docs/README.asciidoc

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public SearchAfterSortedDocQuery(Sort sort, FieldDoc after) {
this.sort = Objects.requireNonNull(sort);
this.after = after;
int numFields = sort.getSort().length;
this.fieldComparators = new FieldComparator[numFields];
this.fieldComparators = new FieldComparator<?>[numFields];
this.reverseMuls = new int[numFields];
for (int i = 0; i < numFields; i++) {
SortField sortField = sort.getSort()[i];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,23 @@
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.SearchSortValuesAndFormats;

import static org.elasticsearch.core.Types.forciblyCast;

/**
* Utility class to keep track of the bottom doc's sort values in a distributed search.
*/
class BottomSortValuesCollector {
private final int topNSize;
private final SortField[] sortFields;
private final FieldComparator[] comparators;
private final FieldComparator<?>[] comparators;
private final int[] reverseMuls;

private volatile long totalHits;
private volatile SearchSortValuesAndFormats bottomSortValues;

BottomSortValuesCollector(int topNSize, SortField[] sortFields) {
this.topNSize = topNSize;
this.comparators = new FieldComparator[sortFields.length];
this.comparators = new FieldComparator<?>[sortFields.length];
this.reverseMuls = new int[sortFields.length];
this.sortFields = sortFields;
for (int i = 0; i < sortFields.length; i++) {
Expand Down Expand Up @@ -90,7 +92,7 @@ private FieldDoc extractBottom(TopFieldDocs topDocs) {

private int compareValues(Object[] v1, Object[] v2) {
for (int i = 0; i < v1.length; i++) {
int cmp = reverseMuls[i] * comparators[i].compareValues(v1[i], v2[i]);
int cmp = reverseMuls[i] * comparators[i].compareValues(forciblyCast(v1[i]), forciblyCast(v2[i]));
if (cmp != 0) {
return cmp;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import java.util.stream.IntStream;
import java.util.stream.Stream;

import static org.elasticsearch.core.Types.forciblyCast;

/**
* This search phase can be used as an initial search phase to pre-filter search shards based on query rewriting.
* The queries are rewritten against the shards and based on the rewrite result shards might be able to be excluded
Expand Down Expand Up @@ -185,7 +187,11 @@ private static boolean shouldSortShards(MinAndMax<?>[] minAndMaxes) {
private static Comparator<Integer> shardComparator(GroupShardsIterator<SearchShardIterator> shardsIts,
MinAndMax<?>[] minAndMaxes,
SortOrder order) {
final Comparator<Integer> comparator = Comparator.comparing(index -> minAndMaxes[index], MinAndMax.getComparator(order));
final Comparator<Integer> comparator = Comparator.comparing(
index -> minAndMaxes[index],
forciblyCast(MinAndMax.getComparator(order))
);

return comparator.thenComparing(index -> shardsIts.get(index));
}

Expand All @@ -197,7 +203,7 @@ private static final class CanMatchSearchPhaseResults extends SearchPhaseResults
CanMatchSearchPhaseResults(int size) {
super(size);
possibleMatches = new FixedBitSet(size);
minAndMaxes = new MinAndMax[size];
minAndMaxes = new MinAndMax<?>[size];
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public TransportBroadcastByNodeAction(

private Response newResponse(
Request request,
AtomicReferenceArray responses,
AtomicReferenceArray<?> responses,
List<NoShardAvailableActionException> unavailableShardExceptions,
Map<String, List<ShardRouting>> nodes,
ClusterState clusterState) {
Expand All @@ -126,6 +126,7 @@ private Response newResponse(
exceptions.add(new DefaultShardOperationFailedException(shard.getIndexName(), shard.getId(), exception));
}
} else {
@SuppressWarnings("unchecked")
NodeResponse response = (NodeResponse) responses.get(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does forciblyCast not work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it would, but I don't really like using it - I feel like the annotation draws attention more than a function call, even with its name, and I'd prefer the engineers to want to make such cast unnecessary wherever possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should have some clear guidance here then on when one is preferable over the other. As it is, it doesn't seem obvious to me. My intention behind adding that method was to eliminate the need for this suppressions all over the case when there's no alternative to casting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added something to CONTRIBUTING.md, see what you think.

broadcastByNodeResponses.addAll(response.results);
totalShards += response.getTotalShards();
Expand Down Expand Up @@ -401,7 +402,7 @@ public void messageReceived(final NodeRequest request, TransportChannel channel,
if (logger.isTraceEnabled()) {
logger.trace("[{}] executing operation on [{}] shards", actionName, totalShards);
}
final AtomicArray<Object> shardResultOrExceptions = new AtomicArray(totalShards);
final AtomicArray<Object> shardResultOrExceptions = new AtomicArray<>(totalShards);

final AtomicInteger counter = new AtomicInteger(shards.size());
int shardIndex = -1;
Expand Down Expand Up @@ -430,6 +431,7 @@ public void onFailure(Exception e) {
}
}

@SuppressWarnings("unchecked")
private void finishHim(NodeRequest request, TransportChannel channel, Task task,
AtomicArray<Object> shardResultOrExceptions) {
if (task instanceof CancellableTask) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ protected List<ShardId> shards(Request request, ClusterState clusterState) {

protected abstract ShardRequest newShardRequest(Request request, ShardId shardId);

private void finishAndNotifyListener(ActionListener listener, CopyOnWriteArrayList<ShardResponse> shardsResponses) {
private void finishAndNotifyListener(ActionListener<Response> listener, CopyOnWriteArrayList<ShardResponse> shardsResponses) {
logger.trace("{}: got all shard responses", actionName);
int successfulShards = 0;
int failedShards = 0;
Expand All @@ -159,6 +159,6 @@ private void finishAndNotifyListener(ActionListener listener, CopyOnWriteArrayLi
listener.onResponse(newResponse(successfulShards, failedShards, totalNumCopies, shardFailures));
}

protected abstract BroadcastResponse newResponse(int successfulShards, int failedShards, int totalNumCopies,
protected abstract Response newResponse(int successfulShards, int failedShards, int totalNumCopies,
List<DefaultShardOperationFailedException> shardFailures);
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public SortedSetDocValues ordinals(ValuesHolder values) {
if (multiValued) {
return new MultiDocs(this, values);
} else {
return (SortedSetDocValues) DocValues.singleton(new SingleDocs(this, values));
return DocValues.singleton(new SingleDocs(this, values));
mark-vieira marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public Collection<Accountable> getChildResources() {

@Override
public SortedSetDocValues ordinals(ValuesHolder values) {
return (SortedSetDocValues) DocValues.singleton(new Docs(this, values));
return DocValues.singleton(new Docs(this, values));
}

private static class Docs extends AbstractSortedDocValues {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ protected AbstractGeometryQueryBuilder(String fieldName, Supplier<Geometry> supp
this.fieldName = fieldName;
this.shape = null;
this.supplier = supplier;
this.indexedShapeId = indexedShapeId;;
this.indexedShapeId = indexedShapeId;
}

/**
Expand Down Expand Up @@ -190,6 +190,7 @@ public String fieldName() {
* @param geometry the geometry
* @return this
*/
@SuppressWarnings("unchecked")
public QB shape(Geometry geometry) {
if (geometry == null) {
throw new IllegalArgumentException("No geometry defined");
Expand Down Expand Up @@ -218,6 +219,7 @@ public String indexedShapeId() {
* @param indexedShapeIndex Name of the index where the indexed Shape is
* @return this
*/
@SuppressWarnings("unchecked")
public QB indexedShapeIndex(String indexedShapeIndex) {
this.indexedShapeIndex = indexedShapeIndex;
return (QB)this;
Expand All @@ -237,6 +239,7 @@ public String indexedShapeIndex() {
* @param indexedShapePath Path of the field where the Shape itself is defined
* @return this
*/
@SuppressWarnings("unchecked")
public QB indexedShapePath(String indexedShapePath) {
this.indexedShapePath = indexedShapePath;
return (QB)this;
Expand All @@ -255,6 +258,7 @@ public String indexedShapePath() {
* @param indexedShapeRouting indexed shape routing
* @return this
*/
@SuppressWarnings("unchecked")
public QB indexedShapeRouting(String indexedShapeRouting) {
this.indexedShapeRouting = indexedShapeRouting;
return (QB)this;
Expand All @@ -275,6 +279,7 @@ public String indexedShapeRouting() {
* @param relation relation of the shapes
* @return this
*/
@SuppressWarnings("unchecked")
public QB relation(ShapeRelation relation) {
if (relation == null) {
throw new IllegalArgumentException("No Shape Relation defined");
Expand Down Expand Up @@ -427,6 +432,7 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
}

@Override
@SuppressWarnings("rawtypes")
protected boolean doEquals(AbstractGeometryQueryBuilder other) {
return Objects.equals(fieldName, other.fieldName)
&& Objects.equals(indexedShapeId, other.indexedShapeId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ public Item(@Nullable String index, XContentBuilder doc) {
/**
* Read from a stream.
*/
@SuppressWarnings("unchecked")
Item(StreamInput in) throws IOException {
index = in.readOptionalString();
if (in.getVersion().before(Version.V_8_0_0)) {
Expand Down
Loading