-
Notifications
You must be signed in to change notification settings - Fork 283
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
Abstract waitForInit to minimize duplication and improve test reliability #1935
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,6 @@ | |
import org.junit.Test; | ||
|
||
import org.opensearch.OpenSearchSecurityException; | ||
import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; | ||
import org.opensearch.action.admin.indices.create.CreateIndexRequest; | ||
import org.opensearch.action.admin.indices.create.CreateIndexResponse; | ||
import org.opensearch.client.Client; | ||
|
@@ -67,18 +66,6 @@ public Collection<Object> createComponents(Client client, ClusterService cluster | |
} | ||
} | ||
|
||
//Wait for the security plugin to load roles. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we use that new method inside of SingleCluster to after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In one of the previous iterations I moved this call up to be done directly after |
||
private void waitForInit(Client client) throws Exception { | ||
try { | ||
client.admin().cluster().health(new ClusterHealthRequest()).actionGet(); | ||
} catch (OpenSearchSecurityException ex) { | ||
if(ex.getMessage().contains("OpenSearch Security not initialized")) { | ||
Thread.sleep(500); | ||
waitForInit(client); | ||
} | ||
} | ||
} | ||
|
||
@Test | ||
public void testSecurityUserInjection() throws Exception { | ||
final Settings clusterNodeSettings = Settings.builder() | ||
|
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.
out of curiosity, why did this value change to 10?
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.
This was the source of the flakiness in TenantInfoActionTest. We would often receive build output like this:
See the second to last line for the timeout. By increasing the timeout gives a bit more of a buffer for the configuration load to complete.
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.
okay...got it.