Fix backward compatibility with std visibility when doing search attr…

…ibute operations (#3907)
temporalio · Feb 6, 2023 · 9972e77 · 9972e77
1 parent 157cd11
commit 9972e77
Show file tree

Hide file tree

Showing 8 changed files with 63 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
+			}
 		}
 	}