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:
us...@infra.apache.org
With regards,
Apache Git Services
