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

[Segment Replication] Add test support for segrep integration tests #3818

Closed
dreamer-89 opened this issue Jul 7, 2022 · 5 comments
Closed
Assignees
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request

Comments

@dreamer-89
Copy link
Member

Summary

Creating this issue to track the work needed to have test framework for testing SegRep feature; which is currently an experimental feature and runs behind a feature flag (recent great work from @kartg that allows feature toggle without any code deployment and controlled via system property). Thus, enabling segrep needs a feature flag as pre-requisite. Setting up this feature flag (via System property) may fiddle with remaining tests. Thus, as part of this issue, we need some mechanism which develops segrep tests framework to allow them run without interfering with remaining tests.

Segment Replication
Design Proposal #2229
Meta Issue #2194

@dreamer-89 dreamer-89 added enhancement Enhancement or improvement to existing feature or request untriaged labels Jul 7, 2022
@dreamer-89
Copy link
Member Author

dreamer-89 commented Jul 8, 2022

Proposal

  1. Create a new Test type task which launches tests to run in separate JVM. The definition of task should look something like
task segRepTestTask(type: Test) {
    include 'com/opensearch/**'
    systemProperty 'opensearch.experimental.feature.replication_type.enabled', 'value'
    jvmArgs '-XX:MaxPermSize=256m ...'
}
  1. Use System.setProperty() inside the test class (or parent class) and reset after exit. As our tests are running in parallel today, we need to isolated these tests by using Spock framework. . The test class can be annotated with @Isolated to ensure it runs in isolation with other tests.

@dreamer-89
Copy link
Member Author

@reta @nknize @mch2 : What do you advise on approach for executing segment replication tests ?

@mch2
Copy link
Member

mch2 commented Jul 26, 2022

@dreamer-89 I prefer option 1 of isolating these tests to a separate task where we pass the jvm flag, as long as they are included with check & run in a separate jvm. All ITs for features under FeatureFlags will need the same setup, so maybe we enable all the features defined in that file and isolate tests to a separate package to easily include all ITs? wdyt?

I want to say maybe we could simply turn on the flags for gradle checks, but most users won't use them until release and there is conditional guice binding that needs to be tested.

@Rishikesh1159
Copy link
Member

Rishikesh1159 commented Sep 29, 2022

After discussion with team, we decided not to move forward with any of above mentioned solutions. Instead we decided to change the implementation of feature flags.

Current behavior of Feature flag:

We take feature flags input using jvm args (system property). When starting up node we check if Feature flag is enabled and we load specific classes related to new feature enabled. We have our Feature flag check here and to enable a feature flag we have to pass jvm args : 'opensearch.experimental.feature.replication_type.enabled', 'true'

Problem with current behavior of Feature flags:

We don't have a good way to enable/disable integration tests related to feature flags. If we want to run an integration test related to new feature we have to enable specific feature flag which makes all other integration tests (even those not related to feature we are testing) will be run with Feature flag turned on, sometimes this might lead to incorrect results. If we don't enable feature flag then during ./gradlew check the integration tests related to new feature will never be checked/ran. We want these new feature integration tests to be ran/checked on every ./gradlew check.

So we need a way to only enable feature flag for the integration tests related to the new feature. This should not affect other integration tests which are not related to this feature. Current Behavior for new feature integration tests, with feature flags disabled we will never run our integration test.

Solution Proposed:

We need to figure out a way to change current implementation of feature flag and pass all the feature flags to node environment and make a node pick those feature flags from environment during it's startup. This way when writing integration tests for new features we can start a node with specific feature flag and drop the node after the integration test is done. This way we won't affect/disrupt other integration tests. And the integration tests related to new feature can also checked/ran for every ./gradlew check

@dreamer-89
Copy link
Member Author

Closing this issue as feature flag settings moved to node config as part of #4102.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed framework enhancement Enhancement or improvement to existing feature or request
Projects
Status: Done
Development

No branches or pull requests

5 participants