ES-10063 Add multi-project support for more stats APIs #127650
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ | |
| import org.elasticsearch.cluster.metadata.IndexMetadata; | ||
| import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; | ||
| import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.ResolvedExpression; | ||
| import org.elasticsearch.cluster.metadata.ProjectId; | ||
| import org.elasticsearch.cluster.metadata.ProjectMetadata; | ||
| import org.elasticsearch.cluster.node.DiscoveryNode; | ||
| import org.elasticsearch.cluster.project.ProjectResolver; | ||
|
|
@@ -83,6 +84,7 @@ | |
| import org.elasticsearch.env.ShardLockObtainFailedException; | ||
| import org.elasticsearch.gateway.MetaStateService; | ||
| import org.elasticsearch.gateway.MetadataStateFormat; | ||
| import org.elasticsearch.index.AbstractIndexComponent; | ||
| import org.elasticsearch.index.CloseUtils; | ||
| import org.elasticsearch.index.Index; | ||
| import org.elasticsearch.index.IndexMode; | ||
|
|
@@ -482,7 +484,13 @@ public NodeIndicesStats stats(CommonStatsFlags flags, boolean includeShardsStats | |
| } | ||
| } | ||
|
|
||
| return new NodeIndicesStats( | ||
| commonStats, | ||
| statsByIndex(this, flags), | ||
| statsByShard(this, flags), | ||
| projectsByIndex(), | ||
| includeShardsStats | ||
| ); | ||
| } | ||
|
|
||
| static Map<Index, CommonStats> statsByIndex(final IndicesService indicesService, final CommonStatsFlags flags) { | ||
|
|
@@ -564,6 +572,13 @@ IndexShardStats indexShardStats(final IndicesService indicesService, final Index | |
| ); | ||
| } | ||
|
|
||
| private Map<Index, ProjectId> projectsByIndex() { | ||
| return indices.values() | ||
| .stream() | ||
| .map(AbstractIndexComponent::index) | ||
| .collect(Collectors.toMap(index -> index, index -> clusterService.state().metadata().projectFor(index).id())); | ||
nielsbauman marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Thinking out loud: by using There was a problem hiding this comment. Ugh. That's a good question. Looking at the creation flow, I believe that creating the At any rate, I think that good defensive coding principles dictate that we should make things work as well as possible in these circumstances. I think the best option might be to switch to using I don't like that, but I'm struggling to think of any better alternatives right now. It would be simple if the The refactoring to ensure that things happen in a different order which guarantees it would be okay is waaaaay to large and risky to justify. So I'm going with the There was a problem hiding this comment.
N.B. If we go with the thing I called option 3 in the other thread, this comment will not be true. The only way that I think this the index could be missing from the map is if we hit this rare timing issue. So I think the question becomes: Do we want to do the plumbing to make an (There is, incidentally, a risk that the stats call could fail if there are two indices with the same name in different projects both of which hit this rare timing issue at the same time, causing a key collision. I'm inclined not to worry about that, as it would be transient, although it could be avoided if we just add a counter to the prefix.) |
||
| } | ||
|
|
||
| /** | ||
| * Checks if changes (adding / removing) indices, shards and so on are allowed. | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| import org.elasticsearch.action.admin.indices.stats.CommonStats; | ||
| import org.elasticsearch.action.admin.indices.stats.IndexShardStats; | ||
| import org.elasticsearch.action.admin.indices.stats.ShardStats; | ||
| import org.elasticsearch.cluster.metadata.Metadata; | ||
| import org.elasticsearch.cluster.metadata.ProjectId; | ||
| import org.elasticsearch.common.collect.Iterators; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
|
|
@@ -55,6 +57,8 @@ | |
| import java.util.Map; | ||
| import java.util.Objects; | ||
|
|
||
| import static java.util.Objects.requireNonNull; | ||
|
|
||
| /** | ||
| * Global information on indices stats running on a specific node. | ||
| */ | ||
|
|
@@ -66,6 +70,7 @@ public class NodeIndicesStats implements Writeable, ChunkedToXContent { | |
| private final CommonStats stats; | ||
| private final Map<Index, List<IndexShardStats>> statsByShard; | ||
| private final Map<Index, CommonStats> statsByIndex; | ||
| private final Map<Index, ProjectId> projectsByIndex; | ||
|
There was a problem hiding this comment. It's a shame we have to pass in this map ourselves (and (de)serialize it), as it's pretty much just the There was a problem hiding this comment. I don't think it's a lot more. It's adding one There was a problem hiding this comment. Ah right, the serialization works a bit different in this class, it doesn't just serialize a full map. Hm ok that makes it a lot less worse, indeed. There was a problem hiding this comment. Oh I just realized I'm mixing things up. I was thinking of the serialization of There was a problem hiding this comment. FWIW, it's every index on the node. I still think that |
||
|
|
||
| public NodeIndicesStats(StreamInput in) throws IOException { | ||
| stats = new CommonStats(in); | ||
|
|
@@ -87,20 +92,31 @@ public NodeIndicesStats(StreamInput in) throws IOException { | |
| } else { | ||
| statsByIndex = new HashMap<>(); | ||
| } | ||
|
|
||
| if (in.getTransportVersion().onOrAfter(TransportVersions.NODES_STATS_SUPPORTS_MULTI_PROJECT)) { | ||
| projectsByIndex = in.readMap(Index::new, ProjectId::readFrom); | ||
| } else { | ||
| projectsByIndex = Map.of(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Constructs an instance. If {@code projectsByIndex} argument is non-null, then the index-to-project map will be stored, and the | ||
| * project IDs will be prepended to the index names when converting this instance to XContent (except when it is the default project). | ||
nielsbauman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| public NodeIndicesStats( | ||
| CommonStats oldStats, | ||
| Map<Index, CommonStats> statsByIndex, | ||
| Map<Index, List<IndexShardStats>> statsByShard, | ||
| Map<Index, ProjectId> projectsByIndex, | ||
|
There was a problem hiding this comment. Nice, I like this approach of passing in the index to project map |
||
| boolean includeShardsStats | ||
| ) { | ||
| if (includeShardsStats) { | ||
| this.statsByShard = requireNonNull(statsByShard); | ||
| } else { | ||
| this.statsByShard = EMPTY_STATS_BY_SHARD; | ||
| } | ||
| this.statsByIndex = requireNonNull(statsByIndex); | ||
|
|
||
| // make a total common stats from old ones and current ones | ||
| this.stats = oldStats; | ||
|
|
@@ -114,6 +130,7 @@ public NodeIndicesStats( | |
| for (CommonStats indexStats : statsByIndex.values()) { | ||
| stats.add(indexStats); | ||
| } | ||
| this.projectsByIndex = requireNonNull(projectsByIndex); | ||
| } | ||
|
|
||
| @Nullable | ||
|
|
@@ -228,19 +245,25 @@ public void writeTo(StreamOutput out) throws IOException { | |
| if (out.getTransportVersion().onOrAfter(VERSION_SUPPORTING_STATS_BY_INDEX)) { | ||
| out.writeMap(statsByIndex); | ||
| } | ||
| if (out.getTransportVersion().onOrAfter(TransportVersions.NODES_STATS_SUPPORTS_MULTI_PROJECT)) { | ||
| out.writeMap(projectsByIndex); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
| NodeIndicesStats that = (NodeIndicesStats) o; | ||
| return stats.equals(that.stats) | ||
| && statsByShard.equals(that.statsByShard) | ||
| && statsByIndex.equals(that.statsByIndex) | ||
| && projectsByIndex.equals(that.projectsByIndex); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(stats, statsByShard, statsByIndex, projectsByIndex); | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -260,7 +283,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP | |
| case INDICES -> ChunkedToXContentHelper.object( | ||
| Fields.INDICES, | ||
| Iterators.map(createCommonStatsByIndex().entrySet().iterator(), entry -> (builder, params) -> { | ||
| builder.startObject(xContentKey(entry.getKey())); | ||
| entry.getValue().toXContent(builder, outerParams); | ||
| return builder.endObject(); | ||
| }) | ||
|
|
@@ -271,7 +294,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP | |
| Iterators.flatMap( | ||
| statsByShard.entrySet().iterator(), | ||
| entry -> ChunkedToXContentHelper.array( | ||
| xContentKey(entry.getKey()), | ||
| Iterators.flatMap( | ||
| entry.getValue().iterator(), | ||
| indexShardStats -> Iterators.concat( | ||
|
|
@@ -291,6 +314,17 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP | |
| ); | ||
| } | ||
|
|
||
| private String xContentKey(Index index) { | ||
| if (projectsByIndex.isEmpty()) { // mapping is not available if this instance came from an older node | ||
| return index.getName(); | ||
| } | ||
| ProjectId projectId = projectsByIndex.get(index); | ||
| if (Objects.equals(projectId, Metadata.DEFAULT_PROJECT_ID)) { | ||
| return index.getName(); | ||
| } | ||
|
There was a problem hiding this comment. I'm a little bit on the fence about this approach. Thinking out loud: on the one hand, this maintains BwC with little effort. On the other hand, if we ever have indices in the default project this'll be a bit off. It's been mentioned a handful of times that we might want to consider (so, super hypothetically) having a shared cluster-level GeoIP index to avoid downloading the same DB over again. We shouldn't make this decision purely based on the GeoIP index, but the idea of the faint possibility of cluster-level indices is something to think about. At the same time, this approach doesn't "break" anything in that case either, it just makes the output inconsistent. So, we either try to account for that situation proactively now, or we decide to go with this approach and we solve that issue if/when it comes. Any thoughts? There was a problem hiding this comment. I guess the options are:
What I've currently implement is option 2. I think this is certainly better than option 1. But if we're seriously considering having indices in the default project when in MP mode (which I hadn't realized was a possibility) and if we think that people might not realize that no prefix means default (which I'm not sure about) then we could switch to option 3 easily enough. What do you think? There was a problem hiding this comment. On reflection, I think I'm leaning towards option 3. This would also mean that, when deserializing an instance from an older node, we could make the map be null and so not show project prefixes: I think this would be valid, because I assume that we will never have to support BWC with nodes older than the current There was a problem hiding this comment. I think, ideally, I'd want to go with option 3 too (not showing the project ID in single-project mode and showing the project ID for all indices in multi-project mode), for consistency. But let's park this discussion for a bit until we resolve #127650 (comment) (or choose not to resolve it and live with it). There was a problem hiding this comment. I just pushed a commit switching to option 3. I know you said to park it, but it's an easy change, and I think it might help the other discussion (esp. for folks who haven't seen the PR before we raise it) if we're looking at the closest we have to a proposal. There was a problem hiding this comment. Oh, ugh, it now occurs to me: There was a problem hiding this comment. I'm wondering whether XContent params might be an option (for both). I've not used those so I don't know what limitations there might be. |
||
| return projectId + "/" + index.getName(); | ||
| } | ||
|
|
||
| private Map<Index, CommonStats> createCommonStatsByIndex() { | ||
| Map<Index, CommonStats> statsMap = new HashMap<>(); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.