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.

Sign up for GitHub

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

Detecting when a deserialized Map is immutable before changing it in IndexDiskUsageStats #77219

Merged
merged 3 commits into from
Sep 7, 2021
Merged

Detecting when a deserialized Map is immutable before changing it in IndexDiskUsageStats #77219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6824f0b
Detecting when a deserialized Map is immutable before changing it in …
masseyke Sep 2, 2021
c95dc5a
making sure fields mutable whenever it is deserialized
masseyke Sep 3, 2021
0ddfa84
Merge branch 'master' into fix/catch-UnsupportedOperationException-in…
elasticmachine Sep 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions ...r/src/main/java/org/elasticsearch/action/admin/indices/diskusage/IndexDiskUsageStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public final class IndexDiskUsageStats implements ToXContentFragment, Writeable
public static final String STORE_SIZE = "store_size";
public static final String STORE_SIZE_IN_BYTES = "store_size_in_bytes";

private final Map<String, PerFieldDiskUsage> fields;
private Map<String, PerFieldDiskUsage> fields;
private long indexSizeInBytes;

public IndexDiskUsageStats(long indexSizeInBytes) {
Expand Down Expand Up @@ -87,7 +87,15 @@ private void checkByteSize(long bytes) {

private PerFieldDiskUsage getOrAdd(String fieldName) {
Objects.requireNonNull(fieldName, "fieldName must be non-null");
return fields.computeIfAbsent(fieldName, k -> new PerFieldDiskUsage());
try {
return fields.computeIfAbsent(fieldName, k -> new PerFieldDiskUsage());
} 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<>();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good point. I'm assuming that this is called infrequently enough that the minor performance hit is not going to matter, right?

return fields.computeIfAbsent(fieldName, k -> new PerFieldDiskUsage());
}
}

public void addInvertedIndex(String fieldName, long bytes) {
Expand Down
37 changes: 37 additions & 0 deletions .../test/java/org/elasticsearch/action/admin/indices/diskusage/IndexDiskUsageStatsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* 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.
*/

package org.elasticsearch.action.admin.indices.diskusage;

import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;

public class IndexDiskUsageStatsTests extends ESTestCase {

public void testEmptySerialization() throws IOException {
IndexDiskUsageStats emptyDndexDiskUsageStats = createEmptyDiskUsageStats();
try (BytesStreamOutput out = new BytesStreamOutput()) {
emptyDndexDiskUsageStats.writeTo(out);
try (StreamInput in = out.bytes().streamInput()) {
IndexDiskUsageStats deserializedNodeStats = new IndexDiskUsageStats(in);
assertEquals(emptyDndexDiskUsageStats.getIndexSizeInBytes(), deserializedNodeStats.getIndexSizeInBytes());
assertEquals(emptyDndexDiskUsageStats.getFields(), deserializedNodeStats.getFields());
// Now just making sure that an exception doesn't get thrown here:
deserializedNodeStats.addStoredField(randomAlphaOfLength(10), randomNonNegativeLong());
}
}
}

private IndexDiskUsageStats createEmptyDiskUsageStats() {
return new IndexDiskUsageStats(randomNonNegativeLong());
}

}