IGNITE-28907 Support merging QueryEntity metadata in CacheConfiguration #13583
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,261 @@ | ||||||
| /* | ||||||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||||||
| * contributor license agreements. See the NOTICE file distributed with | ||||||
| * this work for additional information regarding copyright ownership. | ||||||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||||||
| * (the "License"); you may not use this file except in compliance with | ||||||
| * the License. You may obtain a copy of the License at | ||||||
| * | ||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| * | ||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| * See the License for the specific language governing permissions and | ||||||
| * limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package org.apache.ignite.internal.processors.query; | ||||||
|
|
||||||
| import java.util.ArrayList; | ||||||
| import java.util.Collection; | ||||||
| import java.util.HashMap; | ||||||
| import java.util.LinkedHashMap; | ||||||
| import java.util.LinkedHashSet; | ||||||
| import java.util.List; | ||||||
| import java.util.Map; | ||||||
| import java.util.Objects; | ||||||
| import java.util.Set; | ||||||
| import javax.cache.CacheException; | ||||||
| import org.apache.ignite.cache.QueryEntity; | ||||||
| import org.apache.ignite.cache.QueryIndex; | ||||||
| import org.apache.ignite.internal.util.typedef.F; | ||||||
|
|
||||||
| /** Utility for merging compatible {@link QueryEntity} metadata. */ | ||||||
| public final class QueryEntityMerger { | ||||||
| /** */ | ||||||
| public static final String CONFLICT_MESSAGE_TEMPLATE = "Failed to merge query entities due to conflicting metadata " + | ||||||
| "[cacheName=%s, property=%s, existingValue=%s, incomingValue=%s]"; | ||||||
|
|
||||||
| /** */ | ||||||
| private final String cacheName; | ||||||
|
|
||||||
| /** */ | ||||||
| private QueryEntityMerger(String cacheName) { | ||||||
| this.cacheName = cacheName; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Merges incoming query entity metadata into existing entity. | ||||||
| * | ||||||
| * @param cacheName Cache name. | ||||||
| * @param existing Existing query entity. | ||||||
| * @param incoming Incoming query entity. | ||||||
| * @return Merged query entity. | ||||||
| * @throws CacheException If entities contain conflicting metadata. | ||||||
| */ | ||||||
| public static QueryEntity merge(String cacheName, QueryEntity existing, QueryEntity incoming) { | ||||||
| return new QueryEntityMerger(cacheName).merge0(existing, incoming); | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private QueryEntity merge0(QueryEntity ex, QueryEntity in) { | ||||||
| if (!Objects.equals(ex.findValueType(), in.findValueType())) { | ||||||
| throw new CacheException( | ||||||
| "Failed to merge query entities because value types differ " + | ||||||
| "[cacheName=" + cacheName + | ||||||
| ", existingValueType=" + ex.findValueType() + | ||||||
| ", incomingValueType=" + in.findValueType() + ']' | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| QueryEntity res = new QueryEntity(ex); | ||||||
|
Contributor
Author
There was a problem hiding this comment. I reproduced the The reported problem is real: with the current PR implementation, However, after tracing the actual lifecycle, it looks like the schema-add case does not require defining merge rules for two different The reproducer is
The schema-add lifecycle patches the cache configuration twice: Initially I expected this to be a legitimate merge of two extended entities, but it is not. The first and the second patch operate on different So the "merge" in this scenario is actually an accidentally repeated usage of the same schema-add entity. It is not merging two independently defined I added an isolated regression test for the underlying issue: The test prepares an On assertTrue(oldCfg.getQueryEntities().isEmpty());because both configurations share the same mutable After making More importantly, after this change I also checked whether we have a supported runtime path that would require resolving a real conflict between two different
Again, I also reviewed the internal Therefore I did not add rules for merging conflicting
It is not clear whether they should be conflicts, whether one side should overtake or whether some boolean fields should be combined. I could not find a supported runtime lifecycle that requires making those decisions. So the fix I propose is:
If there is a supported path where two independently created
Contributor
There was a problem hiding this comment. probably it`s better to rewrite like : |
||||||
|
|
||||||
| res.setKeyType(mergeKeyType(ex, in)); | ||||||
|
|
||||||
| res.setValueType(mergeProperty("valueType", ex.getValueType(), in.getValueType())); | ||||||
| res.setTableName(mergeProperty("tableName", ex.getTableName(), in.getTableName())); | ||||||
| res.setKeyFieldName(mergeProperty("keyFieldName", ex.getKeyFieldName(), in.getKeyFieldName())); | ||||||
| res.setValueFieldName(mergeProperty("valueFieldName", ex.getValueFieldName(), in.getValueFieldName())); | ||||||
|
|
||||||
| res.setFields(mergeFields(ex.getFields(), in.getFields())); | ||||||
|
|
||||||
| res.setKeyFields(mergeSet(ex.getKeyFields(), in.getKeyFields())); | ||||||
| res.setNotNullFields(mergeSet(ex.getNotNullFields(), in.getNotNullFields())); | ||||||
|
|
||||||
| res.setAliases(mergeMap("aliases", ex.getAliases(), in.getAliases())); | ||||||
| res.setDefaultFieldValues(mergeMap("defaultFieldValues", ex.getDefaultFieldValues(), in.getDefaultFieldValues())); | ||||||
| res.setFieldsPrecision(mergeMap("fieldsPrecision", ex.getFieldsPrecision(), in.getFieldsPrecision())); | ||||||
| res.setFieldsScale(mergeMap("fieldsScale", ex.getFieldsScale(), in.getFieldsScale())); | ||||||
|
|
||||||
| res.setIndexes(mergeIndexes(res, ex.getIndexes(), in.getIndexes())); | ||||||
|
|
||||||
| return res; | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private String mergeKeyType(QueryEntity ex, QueryEntity in) { | ||||||
| String exKeyType = ex.findKeyType(); | ||||||
| String inKeyType = in.findKeyType(); | ||||||
|
|
||||||
| if (exKeyType != null && inKeyType != null && !Objects.equals(exKeyType, inKeyType)) { | ||||||
| throw mergeConflict( | ||||||
| "keyType", | ||||||
| exKeyType, | ||||||
| inKeyType | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| return ex.getKeyType() != null ? ex.getKeyType() : in.getKeyType(); | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private <T> T mergeProperty(String propName, T existingVal, T incomingVal) { | ||||||
| if (existingVal == null) | ||||||
| return incomingVal; | ||||||
|
|
||||||
| if (incomingVal == null) | ||||||
| return existingVal; | ||||||
|
|
||||||
| if (Objects.equals(existingVal, incomingVal)) | ||||||
| return existingVal; | ||||||
|
|
||||||
| throw mergeConflict(propName, existingVal, incomingVal); | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private LinkedHashMap<String, String> mergeFields( | ||||||
| Map<String, String> existingFields, | ||||||
| Map<String, String> incomingFields | ||||||
| ) { | ||||||
| if (existingFields == null && incomingFields == null) | ||||||
| return null; | ||||||
|
Contributor
There was a problem hiding this comment. I see that result of this operation is used in : org.apache.ignite.cache.QueryEntity#setFields but there is no |
||||||
|
|
||||||
| LinkedHashMap<String, String> res = new LinkedHashMap<>(); | ||||||
|
|
||||||
| if (existingFields != null) | ||||||
| res.putAll(existingFields); | ||||||
|
|
||||||
| if (incomingFields == null) | ||||||
| return res; | ||||||
|
|
||||||
| for (Map.Entry<String, String> entry : incomingFields.entrySet()) { | ||||||
| String field = entry.getKey(); | ||||||
| String incomingType = entry.getValue(); | ||||||
|
|
||||||
| if (!res.containsKey(field)) { | ||||||
| res.put(field, incomingType); | ||||||
|
|
||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| String existingType = res.get(field); | ||||||
|
|
||||||
| if (!Objects.equals(existingType, incomingType)) | ||||||
| throw mergeConflict("fieldType[" + field + ']', existingType, incomingType); | ||||||
| } | ||||||
|
|
||||||
| return res; | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private <T> Map<String, T> mergeMap(String propName, Map<String, T> existingVals, Map<String, T> incomingVals) { | ||||||
| if (existingVals == null && incomingVals == null) | ||||||
| return null; | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
|
|
||||||
| Map<String, T> res = new HashMap<>(); | ||||||
|
|
||||||
| if (existingVals != null) | ||||||
| res.putAll(existingVals); | ||||||
|
|
||||||
| if (incomingVals == null) | ||||||
| return res; | ||||||
|
|
||||||
| for (Map.Entry<String, T> entry : incomingVals.entrySet()) { | ||||||
| String field = entry.getKey(); | ||||||
| T incomingVal = entry.getValue(); | ||||||
|
|
||||||
| if (!res.containsKey(field)) { | ||||||
| res.put(field, incomingVal); | ||||||
|
|
||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| T existingVal = res.get(field); | ||||||
|
|
||||||
| if (!Objects.equals(existingVal, incomingVal)) | ||||||
| throw mergeConflict(propName + '[' + field + ']', existingVal, incomingVal); | ||||||
| } | ||||||
|
|
||||||
| return res; | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private <T> Set<T> mergeSet(Set<T> existing, Set<T> incoming) { | ||||||
| if (F.isEmpty(existing) && F.isEmpty(incoming)) | ||||||
| return null; | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
|
|
||||||
| Set<T> res = new LinkedHashSet<>(); | ||||||
|
|
||||||
| if (existing != null) | ||||||
| res.addAll(existing); | ||||||
|
|
||||||
| if (incoming != null) | ||||||
| res.addAll(incoming); | ||||||
|
|
||||||
| return res; | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private Collection<QueryIndex> mergeIndexes( | ||||||
| QueryEntity entity, | ||||||
| Collection<QueryIndex> existingIndexes, | ||||||
| Collection<QueryIndex> incomingIndexes | ||||||
| ) { | ||||||
| if (F.isEmpty(existingIndexes) && F.isEmpty(incomingIndexes)) | ||||||
| return null; | ||||||
|
Contributor
There was a problem hiding this comment.
Suggested change
|
||||||
|
|
||||||
| List<QueryIndex> res = new ArrayList<>(); | ||||||
|
|
||||||
| Map<String, QueryIndex> indexesByName = new HashMap<>(); | ||||||
|
|
||||||
| if (existingIndexes != null) { | ||||||
| for (QueryIndex idx : existingIndexes) { | ||||||
| String idxName = QueryUtils.indexName(entity, idx); | ||||||
|
|
||||||
| res.add(idx); | ||||||
|
|
||||||
| indexesByName.put(idxName, idx); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if (incomingIndexes == null) | ||||||
| return res; | ||||||
|
|
||||||
| for (QueryIndex incomingIdx : incomingIndexes) { | ||||||
| String idxName = QueryUtils.indexName(entity, incomingIdx); | ||||||
|
|
||||||
| QueryIndex existingIdx = indexesByName.get(idxName); | ||||||
|
|
||||||
| if (existingIdx == null) { | ||||||
| res.add(incomingIdx); | ||||||
|
|
||||||
| indexesByName.put(idxName, incomingIdx); | ||||||
|
|
||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
| if (!existingIdx.equals(incomingIdx)) | ||||||
| throw mergeConflict("index[" + idxName + ']', existingIdx, incomingIdx); | ||||||
| } | ||||||
|
|
||||||
| return res; | ||||||
| } | ||||||
|
|
||||||
| /** */ | ||||||
| private CacheException mergeConflict(String propName, Object existingVal, Object incomingVal) { | ||||||
| return new CacheException( | ||||||
| String.format(CONFLICT_MESSAGE_TEMPLATE, cacheName, propName, existingVal, incomingVal) | ||||||
| ); | ||||||
| } | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I plan to use JSpecify Nullaway frameqork in future, thus we need to be accurate with return types if function is not annotated with
@Nullableplz fix it.