Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Elasticsearch index creation in functional tests #3954

Merged

Conversation

alexshtin
Copy link
Member

What changed?
Fix Elasticsearch index creation in functional tests.

Why?
Recreation of index led to

--- FAIL: TestAdvVisCrossDCTestSuite (78.78s)
    util.go:80: 
        	Error Trace:	/temporal/tests/xdc/util.go:80
        	            				/temporal/tests/xdc/advanced_visibility_test.go:158
        	            				/temporal/tests/xdc/suite.go:132
        	            				/temporal/tests/xdc/advanced_visibility_test.go:83
        	Error:      	Received unexpected error:
        	            	elastic: Error 400 (Bad Request): index [test-visibility-4b3e75-/jZ8ZshDkRCabEESDmKTBSQ] already exists [type=resource_already_exists_exception]
        	Test:       	TestAdvVisCrossDCTestSuite

flaky error.

How did you test it?
Run tests.

Potential risks
No risks.

Is hotfix candidate?
No.

@alexshtin alexshtin requested a review from a team as a code owner February 14, 2023 03:14
@alexshtin alexshtin force-pushed the fix/functional-tests-create-index branch from f66832b to fdd452e Compare February 14, 2023 04:04
Copy link
Contributor

@MichaelSnowden MichaelSnowden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@alexshtin alexshtin force-pushed the fix/functional-tests-create-index branch 3 times, most recently from 15ab8b0 to 2c109de Compare February 14, 2023 21:02
@alexshtin alexshtin force-pushed the fix/functional-tests-create-index branch from 2c109de to e2031fe Compare February 14, 2023 21:06
return err
}
if exists {
logger.Info("Index already exists.", tag.ESIndex(esConfig.GetVisibilityIndex()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger.Warn maybe? It seems that it should never happen.
However, I did see sometimes integration tests failing because index already existed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I initially tried to address in this PR. It shouldn't happen in CI anymore. But locally, in debugging, if you stop debugger, for example, tests are not teared down properly.

@@ -252,6 +258,70 @@ func NewCluster(options *TestClusterConfig, logger log.Logger) (*TestCluster, er
return &TestCluster{testBase: testBase, archiverBase: archiverBase, host: cluster}, nil
}

func setupIndex(esConfig *esclient.Config, logger log.Logger) error {
esClient, err := esclient.NewIntegrationTestsClient(esConfig, logger)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between NewIntegrationTestsClient and NewClient? L180 above uses the second one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewIntegrationTestsClient is more like admin client which supports operation that are needed for tests only to do operations which are normally made outside of server (index template creation, index creation, etc). I have TODO there to properly separate them or, vice versa, merge in one.

@alexshtin alexshtin merged commit 0a22e1c into temporalio:master Feb 14, 2023
@alexshtin alexshtin deleted the fix/functional-tests-create-index branch February 14, 2023 22:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants