Fixing index mode serialization in stats#149391
Conversation
elasticsearchmachine
added
the
Team:Search Relevance
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
| public Map<IndexMode, IndexStats> stats() { | ||
| final Map<IndexMode, IndexStats> stats = new EnumMap<>(IndexMode.class); | ||
| for (IndexMode mode : IndexMode.values()) { | ||
| for (IndexMode mode : IndexMode.availableModes()) { |
There was a problem hiding this comment.
we shouldn't add a bunch of index modes that won't actually be serializable by other nodes because of a feature flag right?
| static Map<IndexMode, IndexStats> getStatsWithoutCache(IndicesService indicesService) { | ||
| Map<IndexMode, IndexStats> stats = new EnumMap<>(IndexMode.class); | ||
| for (IndexMode mode : IndexMode.values()) { | ||
| for (IndexMode mode : IndexMode.availableModes()) { |
There was a problem hiding this comment.
Can be for in a follow up PR:
what happens with BWC stats tests in snapshot builds where all modes are available or when COLUMNAR or VECTORDB comes out of FF?
It looks to me that during rolling upgrade we will still have this error?
There was a problem hiding this comment.
@mayya-sharipova yeah, I am not sure we get away with this unless we have cluster features that are enabled and protecting serialization of stats. I opted for the feature flag route here, but maybe this needs stronger TV serialization protection.
| stats.put(indexMode, new IndexStats()); | ||
| } | ||
| final DiscoveryNode node = DiscoveryNodeUtils.create("node"); | ||
| final var nodeResponse = new IndexModeStatsActionType.NodeResponse(node, stats); |
There was a problem hiding this comment.
Can we use different transport versions to exercise BWC behaviour:
final TransportVersion olderVersion = TransportVersionUtils.getPreviousVersion(
IndexMode.VECTORDB_DOCUMENT_INDEX_MODE, true
);
try (BytesStreamOutput out = new BytesStreamOutput()) {
out.setTransportVersion(olderVersion);
There was a problem hiding this comment.
I think this will still fail. Maybe we need stronger tv serialization protection. But my focus here was ensuring we don't do the silly thing and initialize with feature flagged items by default (which is confusing and wrong).
mayya-sharipova
left a comment
mayya-sharipova
left a comment
There was a problem hiding this comment.
Thanks Ben, it is good enough to unblock release.
I will follow up with an issue to add more protection of serialization of stats.
…nwtrent/elasticsearch into bugfix-only-serialize-available-modes
benwtrent
added
the
auto-merge-without-approval
5 participants
It seems like we are serializing index modes that are protected behind a feature flag. This doesn't seem right and it has caused some weirdness in rolling upgrades where stats calls will fail with the following, though no index has the mode set and the cluster never had the feature flag enabled: