You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by GitBox <gi...@apache.org> on 2020/03/27 01:33:07 UTC
[GitHub] [flume] AndreLouisCaron opened a new pull request #320: Fix issues
reported by SpotBugs in ElasticSearch sink
AndreLouisCaron opened a new pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320
I'm working on some minor contributions to the ElasticSearch sink and I noticed that `mvn install -pl flume-ng-sinks/flume-ng-elasticsearch-sink` fails out of the box due to issues reported by SpotBugs.
In this pull request, I am fixing said issues to keep future diffs clean when contributing changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron closed pull request #320: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron closed pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398988455
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java
##########
@@ -419,7 +413,7 @@ private long parseTTL(String ttl) {
} else if (matcher.group(2).equals("d")) {
return TimeUnit.DAYS.toMillis(Integer.parseInt(matcher.group(1)));
} else if (matcher.group(2).equals("w")) {
- return TimeUnit.DAYS.toMillis(7 * Integer.parseInt(matcher.group(1)));
Review comment:
Note to reviewers: SpotBugs was reporting `ICAST_INTEGER_MULTIPLY_CAST_TO_LONG`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398988763
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchClientFactory.java
##########
@@ -71,7 +71,6 @@ public ElasticSearchClient getLocalClient(String clientType,
return new ElasticSearchTransportClient(serializer);
} else if (clientType.equalsIgnoreCase(TransportClient) && indexBuilder != null) {
return new ElasticSearchTransportClient(indexBuilder);
- } else if (clientType.equalsIgnoreCase(RestClient)) {
Review comment:
Note to reviewers: SpotBugs was reporting `UCF_USELESS_CONTROL_FLOW`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on issue #320: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on issue #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#issuecomment-610572317
@bessbd Now that the build is fixed, I rebased this onto the latest trunk to get a clean build :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398988938
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java
##########
@@ -305,9 +305,8 @@ public void configure(Context context) {
throw new IllegalArgumentException(serializerClazz
+ " is not an ElasticSearchEventSerializer");
}
- } catch (Exception e) {
- logger.error("Could not instantiate event serializer.", e);
- Throwables.propagate(e);
+ } catch (ClassNotFoundException | InstantiationException | IllegalAccessException e) {
+ throw new RuntimeException("Could not instantiate index name builder.", e);
}
Review comment:
Note to reviewers: SpotBugs was reporitng `REC_CATCH_EXCEPTION`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] bessbd commented on a change in pull request #320: Fix
issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r407703334
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java
##########
@@ -105,7 +105,7 @@ public void addEvent(Event event, IndexNameBuilder indexNameBuilder, String inde
parameters.put(INDEX_OPERATION_NAME, indexParameters);
Gson gson = new Gson();
- synchronized (bulkBuilder) {
Review comment:
Looking at this PR, all changes seem awesome to me, but this one (and the one after - same `synchronized (this)`) seems to alter semantics. What do you think about going one step further and making `bulkBuilder` a `StringBuffer` instead of a `StringBuilder` so that the thread-safety of `StringBuffer` is leveraged?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on issue #320: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on issue #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#issuecomment-609973795
@bessbd I took a quick look. I'm not familiar with Java and the ecosystem in this project, but it looks to me like some kind of static code analysis tool is reporting too many errors in code generated by the Thrift compiler.
I tried manually fixing the code in #324. The solution is clearly less than optimal, but I'm not sure how to do it properly. Let's continue that discussion over there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398989115
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java
##########
@@ -105,7 +105,7 @@ public void addEvent(Event event, IndexNameBuilder indexNameBuilder, String inde
parameters.put(INDEX_OPERATION_NAME, indexParameters);
Gson gson = new Gson();
- synchronized (bulkBuilder) {
Review comment:
Note to reviewers: SpotBugs was reporting `ML_SYNC_ON_FIELD_TO_GUARD_CHANGING_THAT_FIELD`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on issue #320: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on issue #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#issuecomment-611007705
@bessbd @mpercy I rebased this and set `spotbugs.maxAllowedViolations=0`. I have some more changes to fix PMD violations as well.
Do you want those in a new commit on the same PR or should I open a new one for those changes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r408087024
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java
##########
@@ -105,7 +105,7 @@ public void addEvent(Event event, IndexNameBuilder indexNameBuilder, String inde
parameters.put(INDEX_OPERATION_NAME, indexParameters);
Gson gson = new Gson();
- synchronized (bulkBuilder) {
Review comment:
The way I understand it, switching from `synchronized()` to a thread-safe `StringBuffer` would create a bug.
There are 1+ "producer" threads that perform this:
```
synchronized (lock) {
bulkBuilder.append(gson.toJson(parameters));
bulkBuilder.append("\n");
bulkBuilder.append(content.toBytesArray().toUtf8());
bulkBuilder.append("\n");
}
```
And there is one "consumer" thread doing this:
```
synchronized (lock) {
entity = bulkBuilder.toString();
bulkBuilder = new StringBuilder();
}
```
I thought the reason we use `synchronized()` here is to prevent the "consumer" thread from capturing an incomplete sequence of the four calls to `.append()`. Either we get all four `.append()` calls or we get none of them. Otherwise, the bulk insert request would be invalid.
If we were to switch to a thread-safe `StringBuffer` instead, would we preserve that guarantee?
In addition, you get the additional performance hit of synchronizing on each call to `.append()` -- I have not profiled, so I have no idea if this makes a difference in practice.
If `synchronized (this)` is an issue, maybe can switch to a dedicated lock object?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398987871
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchLogStashEventSerializer.java
##########
@@ -126,8 +126,9 @@ private void appendHeaders(XContentBuilder builder, Event event)
}
builder.startObject("@fields");
- for (String key : headers.keySet()) {
- byte[] val = headers.get(key).getBytes(charset);
+ for (Map.Entry<String,String> entry : headers.entrySet()) {
+ String key = entry.getKey();
+ byte[] val = entry.getValue().getBytes(charset);
Review comment:
Note to reviewers: SpotBugs was complaining about `WMI_WRONG_MAP_ITERATOR`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398988603
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/TimestampedEvent.java
##########
@@ -49,7 +49,7 @@
this.timestamp = DateTimeUtils.currentTimeMillis();
headers.put("timestamp", String.valueOf(timestamp ));
} else {
- this.timestamp = Long.valueOf(timestampString);
Review comment:
Note to reviewers: SpotBugs was reporting `DM_BOXED_PRIMITIVE_FOR_PARSING`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron edited a comment on issue #320: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron edited a comment on issue #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#issuecomment-611007705
@bessbd @mpercy I rebased this and set `spotbugs.maxAllowedViolations=0`.
I have some more changes to fix PMD violations as well. Do you want those in a new commit on the same PR or should I open a new one for those changes?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398988243
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java
##########
@@ -331,13 +330,8 @@ public void configure(Context context) {
indexNameBuilder = clazz.newInstance();
indexnameBuilderContext.put(INDEX_NAME, indexName);
indexNameBuilder.configure(indexnameBuilderContext);
- } catch (Exception e) {
- logger.error("Could not instantiate index name builder.", e);
- Throwables.propagate(e);
- }
-
- if (sinkCounter == null) {
- sinkCounter = new SinkCounter(getName());
Review comment:
Note to reviewers: SportBugs was reporting `CN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE` because `sinkCounter` is instantiated twice.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398987831
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchDynamicSerializer.java
##########
@@ -64,9 +64,10 @@ private void appendBody(XContentBuilder builder, Event event)
private void appendHeaders(XContentBuilder builder, Event event)
throws IOException {
Map<String, String> headers = event.getHeaders();
- for (String key : headers.keySet()) {
- ContentBuilderUtil.appendField(builder, key,
- headers.get(key).getBytes(charset));
+ for (Map.Entry<String,String> entry : headers.entrySet()) {
+ String key = entry.getKey();
+ byte[] val = entry.getValue().getBytes(charset);
Review comment:
Note to reviewers: SpotBugs was complaining about `WMI_WRONG_MAP_ITERATOR`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] bessbd commented on issue #320: Fix issues reported by
SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
bessbd commented on issue #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#issuecomment-609886387
Hi @AndreLouisCaron ,
Thank you for the PRs! I've managed to skim through them and they look like valuable changes!
Unfortunately, the build is broken and I don't think I have enough time to fix it nowadays. Would you be interested in looking into that? If you'd need any support, please don't hesitate to let me know.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r398988894
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java
##########
@@ -331,13 +330,8 @@ public void configure(Context context) {
indexNameBuilder = clazz.newInstance();
indexnameBuilderContext.put(INDEX_NAME, indexName);
indexNameBuilder.configure(indexnameBuilderContext);
- } catch (Exception e) {
- logger.error("Could not instantiate index name builder.", e);
- Throwables.propagate(e);
- }
Review comment:
Note to reviewers: SpotBugs was reporitng `REC_CATCH_EXCEPTION`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on a change in pull request #320:
Fix issues reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on a change in pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#discussion_r408087024
##########
File path: flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/client/ElasticSearchRestClient.java
##########
@@ -105,7 +105,7 @@ public void addEvent(Event event, IndexNameBuilder indexNameBuilder, String inde
parameters.put(INDEX_OPERATION_NAME, indexParameters);
Gson gson = new Gson();
- synchronized (bulkBuilder) {
Review comment:
The way I understand it, switching from `synchronized()` to a thread-safe `StringBuffer` would create a bug.
There are 1+ "producer" threads that perform this:
```
synchronized (lock) {
bulkBuilder.append(gson.toJson(parameters));
bulkBuilder.append("\n");
bulkBuilder.append(content.toBytesArray().toUtf8());
bulkBuilder.append("\n");
}
```
And there is one "consumer" thread doing this:
```
synchronized (lock) {
entity = bulkBuilder.toString();
bulkBuilder = new StringBuilder();
}
```
I thought the reason we use `synchronized()` here is to prevent the "consumer" thread from capturing an incomplete sequence of the four calls to `.append()`. Either we get all four `.append()` calls or we get none of them. Otherwise, the bulk insert request would be invalid.
In addition, you get the additional performance hit of synchronizing on each call to `.append()` -- I have not profiled, so I have no idea if this makes a difference in practice.
If `synchronized (this)` is an issue, maybe can switch to a dedicated lock object?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron opened a new pull request #320: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron opened a new pull request #320: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320
I'm working on some minor contributions to the ElasticSearch sink and I noticed that `mvn install -pl flume-ng-sinks/flume-ng-elasticsearch-sink` fails out of the box due to issues reported by SpotBugs.
In this pull request, I am fixing said issues to keep future diffs clean when contributing changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [flume] AndreLouisCaron commented on issue #320: WIP: Fix issues
reported by SpotBugs in ElasticSearch sink
Posted by GitBox <gi...@apache.org>.
AndreLouisCaron commented on issue #320: WIP: Fix issues reported by SpotBugs in ElasticSearch sink
URL: https://github.com/apache/flume/pull/320#issuecomment-610737267
Before merging, I will also set the `spotbugs.maxAllowedViolations` property to 0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services