From 9972e7738a9a1b0b28d699f39c25fdac4d9bacbd Mon Sep 17 00:00:00 2001 From: Rodrigo Zhou Date: Mon, 6 Feb 2023 13:06:39 -0800 Subject: [PATCH] Fix backward compatibility with std visibility when doing search attribute operations (#3907) --- .../standard/cassandra/visibility_store.go | 6 +++--- .../store/standard/sql/visibility_store.go | 4 +++- .../store/standard/visibility_store.go | 4 +++- common/searchattribute/manager.go | 6 +++++- service/frontend/adminHandler.go | 21 ++++++++++++++++--- service/frontend/operator_handler.go | 21 ++++++++++++++++--- service/history/fx.go | 13 +++++++----- tests/test_cluster.go | 10 ++++----- 8 files changed, 63 insertions(+), 22 deletions(-) diff --git a/common/persistence/visibility/store/standard/cassandra/visibility_store.go b/common/persistence/visibility/store/standard/cassandra/visibility_store.go index 2a06f235f1e..218ab9e05f5 100644 --- a/common/persistence/visibility/store/standard/cassandra/visibility_store.go +++ b/common/persistence/visibility/store/standard/cassandra/visibility_store.go @@ -140,7 +140,6 @@ type ( visibilityStore struct { session gocql.Session lowConslevel gocql.Consistency - keyspace string } ) @@ -159,7 +158,6 @@ func NewVisibilityStore( return &visibilityStore{ session: session, lowConslevel: gocql.One, - keyspace: cfg.Keyspace, }, nil } @@ -168,7 +166,9 @@ func (v *visibilityStore) GetName() string { } func (v *visibilityStore) GetIndexName() string { - return v.keyspace + // GetIndexName is used to get cluster metadata, which in verstions < v1.20 + // were stored in an empty string key. + return "" } // Close releases the resources held by this object diff --git a/common/persistence/visibility/store/standard/sql/visibility_store.go b/common/persistence/visibility/store/standard/sql/visibility_store.go index ee6d17b3998..30e7edfdc6b 100644 --- a/common/persistence/visibility/store/standard/sql/visibility_store.go +++ b/common/persistence/visibility/store/standard/sql/visibility_store.go @@ -81,7 +81,9 @@ func (s *visibilityStore) GetName() string { } func (s *visibilityStore) GetIndexName() string { - return s.sqlStore.GetDbName() + // GetIndexName is used to get cluster metadata, which in verstions < v1.20 + // were stored in an empty string key. + return "" } func (s *visibilityStore) RecordWorkflowExecutionStarted( diff --git a/common/persistence/visibility/store/standard/visibility_store.go b/common/persistence/visibility/store/standard/visibility_store.go index c75b193673e..ba21dcaaaf6 100644 --- a/common/persistence/visibility/store/standard/visibility_store.go +++ b/common/persistence/visibility/store/standard/visibility_store.go @@ -74,7 +74,9 @@ func (s *standardStore) GetName() string { } func (s *standardStore) GetIndexName() string { - return s.store.GetIndexName() + // GetIndexName is used to get cluster metadata, which in verstions < v1.20 + // were stored in an empty string key. + return "" } func (s *standardStore) RecordWorkflowExecutionStarted( diff --git a/common/searchattribute/manager.go b/common/searchattribute/manager.go index 9ecf8e6b198..87930f09264 100644 --- a/common/searchattribute/manager.go +++ b/common/searchattribute/manager.go @@ -125,7 +125,11 @@ func (m *managerImpl) GetSearchAttributes( if indexName != "" { indexSearchAttributes, ok = saCache.searchAttributes[""] if ok { - maps.Copy(result.customSearchAttributes, indexSearchAttributes.customSearchAttributes) + if result.customSearchAttributes == nil { + result.customSearchAttributes = maps.Clone(indexSearchAttributes.customSearchAttributes) + } else { + maps.Copy(result.customSearchAttributes, indexSearchAttributes.customSearchAttributes) + } } } return result, nil diff --git a/service/frontend/adminHandler.go b/service/frontend/adminHandler.go index fa82d805be8..ec6b1a1e452 100644 --- a/service/frontend/adminHandler.go +++ b/service/frontend/adminHandler.go @@ -270,7 +270,12 @@ func (adh *AdminHandler) AddSearchAttributes( } } - if adh.visibilityMgr.GetName() == elasticsearch.PersistenceName { + // TODO (rodrigozhou): Remove condition `indexName == ""`. + // If indexName == "", then calling addSearchAttributesElasticsearch will + // register the search attributes in the cluster metadata if ES is up or if + // `skip-schema-update` is set. This is for backward compatibility using + // standard visibility. + if adh.visibilityMgr.GetName() == elasticsearch.PersistenceName || indexName == "" { err = adh.addSearchAttributesElasticsearch(ctx, request, indexName) } else { err = adh.addSearchAttributesSQL(ctx, request, currentSearchAttributes) @@ -404,7 +409,12 @@ func (adh *AdminHandler) RemoveSearchAttributes( } var err error - if adh.visibilityMgr.GetName() == elasticsearch.PersistenceName { + // TODO (rodrigozhou): Remove condition `indexName == ""`. + // If indexName == "", then calling addSearchAttributesElasticsearch will + // register the search attributes in the cluster metadata if ES is up or if + // `skip-schema-update` is set. This is for backward compatibility using + // standard visibility. + if adh.visibilityMgr.GetName() == elasticsearch.PersistenceName || indexName == "" { err = adh.removeSearchAttributesElasticsearch(ctx, request, indexName) } else { err = adh.removeSearchAttributesSQL(ctx, request) @@ -505,7 +515,12 @@ func (adh *AdminHandler) GetSearchAttributes( return nil, serviceerror.NewUnavailable(fmt.Sprintf(errUnableToGetSearchAttributesMessage, err)) } - if adh.visibilityMgr.GetName() == elasticsearch.PersistenceName { + // TODO (rodrigozhou): Remove condition `indexName == ""`. + // If indexName == "", then calling addSearchAttributesElasticsearch will + // register the search attributes in the cluster metadata if ES is up or if + // `skip-schema-update` is set. This is for backward compatibility using + // standard visibility. + if adh.visibilityMgr.GetName() == elasticsearch.PersistenceName || indexName == "" { return adh.getSearchAttributesElasticsearch(ctx, indexName, searchAttributes) } return adh.getSearchAttributesSQL(request, searchAttributes) diff --git a/service/frontend/operator_handler.go b/service/frontend/operator_handler.go index f8a88aa687c..36fe72251bc 100644 --- a/service/frontend/operator_handler.go +++ b/service/frontend/operator_handler.go @@ -192,7 +192,12 @@ func (h *OperatorHandlerImpl) AddSearchAttributes( } } - if h.visibilityMgr.GetName() == elasticsearch.PersistenceName { + // TODO (rodrigozhou): Remove condition `indexName == ""`. + // If indexName == "", then calling addSearchAttributesElasticsearch will + // register the search attributes in the cluster metadata if ES is up or if + // `skip-schema-update` is set. This is for backward compatibility using + // standard visibility. + if h.visibilityMgr.GetName() == elasticsearch.PersistenceName || indexName == "" { err = h.addSearchAttributesElasticsearch(ctx, request, indexName) } else { err = h.addSearchAttributesSQL(ctx, request, currentSearchAttributes) @@ -325,7 +330,12 @@ func (h *OperatorHandlerImpl) RemoveSearchAttributes( var err error indexName := h.visibilityMgr.GetIndexName() - if h.visibilityMgr.GetName() == elasticsearch.PersistenceName { + // TODO (rodrigozhou): Remove condition `indexName == ""`. + // If indexName == "", then calling addSearchAttributesElasticsearch will + // register the search attributes in the cluster metadata if ES is up or if + // `skip-schema-update` is set. This is for backward compatibility using + // standard visibility. + if h.visibilityMgr.GetName() == elasticsearch.PersistenceName || indexName == "" { err = h.removeSearchAttributesElasticsearch(ctx, request, indexName) } else { err = h.removeSearchAttributesSQL(ctx, request) @@ -425,7 +435,12 @@ func (h *OperatorHandlerImpl) ListSearchAttributes( ) } - if h.visibilityMgr.GetName() == elasticsearch.PersistenceName { + // TODO (rodrigozhou): Remove condition `indexName == ""`. + // If indexName == "", then calling addSearchAttributesElasticsearch will + // register the search attributes in the cluster metadata if ES is up or if + // `skip-schema-update` is set. This is for backward compatibility using + // standard visibility. + if h.visibilityMgr.GetName() == elasticsearch.PersistenceName || indexName == "" { return h.listSearchAttributesElasticsearch(ctx, indexName, searchAttributes) } return h.listSearchAttributesSQL(request, searchAttributes) diff --git a/service/history/fx.go b/service/history/fx.go index 7bb72df3f8b..30dc30175c0 100644 --- a/service/history/fx.go +++ b/service/history/fx.go @@ -42,6 +42,9 @@ import ( "go.temporal.io/server/common/metrics" "go.temporal.io/server/common/namespace" persistenceClient "go.temporal.io/server/common/persistence/client" + "go.temporal.io/server/common/persistence/sql/sqlplugin/mysql" + "go.temporal.io/server/common/persistence/sql/sqlplugin/postgresql" + "go.temporal.io/server/common/persistence/sql/sqlplugin/sqlite" "go.temporal.io/server/common/persistence/visibility" "go.temporal.io/server/common/persistence/visibility/manager" "go.temporal.io/server/common/persistence/visibility/store/elasticsearch" @@ -165,11 +168,11 @@ func ConfigProvider( indexName := "" if persistenceConfig.StandardVisibilityConfigExist() { storeConfig := persistenceConfig.DataStores[persistenceConfig.VisibilityStore] - switch { - case storeConfig.Cassandra != nil: - indexName = storeConfig.Cassandra.Keyspace - case storeConfig.SQL != nil: - indexName = storeConfig.SQL.DatabaseName + if storeConfig.SQL != nil { + switch storeConfig.SQL.PluginName { + case mysql.PluginNameV8, postgresql.PluginNameV12, sqlite.PluginName: + indexName = storeConfig.SQL.DatabaseName + } } } else if persistenceConfig.AdvancedVisibilityConfigExist() { indexName = esConfig.GetVisibilityIndex() diff --git a/tests/test_cluster.go b/tests/test_cluster.go index cdf881ab6b4..f650c583d41 100644 --- a/tests/test_cluster.go +++ b/tests/test_cluster.go @@ -177,11 +177,11 @@ func NewCluster(options *TestClusterConfig, logger log.Logger) (*TestCluster, er } } else { storeConfig := pConfig.DataStores[pConfig.VisibilityStore] - switch { - case storeConfig.Cassandra != nil: - indexName = storeConfig.Cassandra.Keyspace - case storeConfig.SQL != nil: - indexName = storeConfig.SQL.DatabaseName + if storeConfig.SQL != nil { + switch storeConfig.SQL.PluginName { + case mysql.PluginNameV8, postgresql.PluginNameV12, sqlite.PluginName: + indexName = storeConfig.SQL.DatabaseName + } } }