Detecting when a deserialized Map is immutable before changing it in IndexDiskUsageStats #77219
Conversation
…IndexDisUsageStats
|
Pinging @elastic/es-data-management (Team:Data Management) |
| } catch (UnsupportedOperationException e) { | ||
| /* This happens if this object had been serialized and deserialized while fields was empty, since StreamInput returns | ||
| * immutable maps for empty maps | ||
| */ | ||
| this.fields = new HashMap<>(); |
There was a problem hiding this comment.
I think this is lossy, because if we were to ever change StreamInput::readMap to return Collections.unmodifiableMap(<some-map-with-data>) then it would be reset back to an empty one here and we would "lose" some of the data.
I think instead, because we know we might be changing it, we should do:
this.fields = new HashMap<>(in.readMap(StreamInput::readString, PerFieldDiskUsage::new));So that it is always available for changing. Then, in future work, we can look at making this class behave in a more immutable style.
There was a problem hiding this comment.
Hmm, good point. I'm assuming that this is called infrequently enough that the minor performance hit is not going to matter, right?
|
@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample |
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine run elasticsearch-ci/part-2 |
|
@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample |
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine update branch |
…-IndexDiskUsageStats
masseyke
deleted the
fix/catch-UnsupportedOperationException-in-IndexDiskUsageStats
branch
|
I think this explains #77327, could it be backported to 7.x and 7.15? |
|
Yeah, I'll backport it. |
…IndexDiskUsageStats (elastic#77219) This commit prevents errors that can happen if an empty fields map is serialized and deserialized in IndexDiskUsageStats.
…IndexDiskUsageStats (elastic#77219) This commit prevents errors that can happen if an empty fields map is serialized and deserialized in IndexDiskUsageStats.
An IT test failed for me with this stack trace:
at org.elasticsearch.ElasticsearchException.guessRootCauses(ElasticsearchException.java:633) at org.elasticsearch.ElasticsearchException.generateFailureXContent(ElasticsearchException.java:561) at org.elasticsearch.rest.BytesRestResponse.build(BytesRestResponse.java:138) at org.elasticsearch.rest.BytesRestResponse.<init>(BytesRestResponse.java:99) at org.elasticsearch.rest.BytesRestResponse.<init>(BytesRestResponse.java:82) at org.elasticsearch.rest.action.RestActionListener.onFailure(RestActionListener.java:55) at org.elasticsearch.rest.action.RestCancellableNodeClient$1.onFailure(RestCancellableNodeClient.java:96) at org.elasticsearch.action.support.TransportAction$1.onFailure(TransportAction.java:92) at org.elasticsearch.action.support.broadcast.TransportBroadcastAction$AsyncBroadcastAction.finishHim(TransportBroadcastAction.java:239) at org.elasticsearch.action.support.broadcast.TransportBroadcastAction$AsyncBroadcastAction.onOperation(TransportBroadcastAction.java:197) at org.elasticsearch.action.support.broadcast.TransportBroadcastAction$AsyncBroadcastAction$1.handleResponse(TransportBroadcastAction.java:178) at org.elasticsearch.action.support.broadcast.TransportBroadcastAction$AsyncBroadcastAction$1.handleResponse(TransportBroadcastAction.java:170) at org.elasticsearch.transport.TransportService$5.handleResponse(TransportService.java:737) at org.elasticsearch.transport.TransportService$ContextRestoreResponseHandler.handleResponse(TransportService.java:1279) at org.elasticsearch.transport.TransportService$DirectResponseChannel.processResponse(TransportService.java:1357) at org.elasticsearch.transport.TransportService$DirectResponseChannel.sendResponse(TransportService.java:1337) at org.elasticsearch.transport.TaskTransportChannel.sendResponse(TaskTransportChannel.java:41) at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:134) at org.elasticsearch.action.ActionRunnable.lambda$supply$0(ActionRunnable.java:47) at org.elasticsearch.action.ActionRunnable$2.doRun(ActionRunnable.java:62) at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:737) at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:748) Caused by: java.lang.UnsupportedOperationException at java.util.Collections$EmptyMap.computeIfAbsent(Collections.java:4631) at org.elasticsearch.action.admin.indices.diskusage.IndexDiskUsageStats.getOrAdd(IndexDiskUsageStats.java:90) at org.elasticsearch.action.admin.indices.diskusage.IndexDiskUsageStats.lambda$add$3(IndexDiskUsageStats.java:124) at java.util.HashMap.forEach(HashMap.java:1289) at org.elasticsearch.action.admin.indices.diskusage.IndexDiskUsageStats.add(IndexDiskUsageStats.java:124) at org.elasticsearch.action.admin.indices.diskusage.TransportAnalyzeIndexDiskUsageAction.lambda$newResponse$0(TransportAnalyzeIndexDiskUsageAction.java:97) at java.util.HashMap.compute(HashMap.java:1197) at org.elasticsearch.action.admin.indices.diskusage.TransportAnalyzeIndexDiskUsageAction.newResponse(TransportAnalyzeIndexDiskUsageAction.java:97) at org.elasticsearch.action.admin.indices.diskusage.TransportAnalyzeIndexDiskUsageAction.newResponse(TransportAnalyzeIndexDiskUsageAction.java:42) at org.elasticsearch.action.support.broadcast.TransportBroadcastAction$AsyncBroadcastAction.finishHim(TransportBroadcastAction.java:237) ... 16 moreIt looks like if an IndexDiskUsageStats object is serialized and then deserialized before anything is written to it, then the deserialized fields Map is immutable. Since this ought to be very rare, and there is no clean way to detect that a Map is immutable, I just catch the exception and create a new mutable Map here.