-
Notifications
You must be signed in to change notification settings - Fork 104
feat(amber): enable user system by default #3782
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
Conversation
…xiaozhen-default-user-sys # Conflicts: # core/amber/src/test/scala/edu/uci/ics/amber/engine/e2e/DataProcessingSpec.scala
|
Do we want to remove the user-system flag directly? Does user-system = false still work after this PR? If it doesn't work, I suggest we just remove it. |
|
Is it possible to include a diagram to visualize the change? |
That would be the next PR. It would be too many changes for this PR. |
I will include a diagram about the behaviors of these test cases in relation to Singleton classes and the databases. |
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.
LGTM. Left one minor comment
|
I will review this PR after PR #3824 is merged. |
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.
LGTM!
# Purpose This PR is a successor of #3782. As the non-user system mode is no longer used or maintained, we can remove the flag to switch between user-system being enabled/disabled, and keep only the mode of user-system being enabled. # Content - Removed the `user-sys.enabled` flag, both in the frontend and backend. - Removed all the if-else statements based on this flag in the codebase. Only the cases of user system being enabled are kept. - Removed `ExecutionResourceMapping` in the backend as it is no longer needed. - Removed `WorkflowCacheService` in the frontend as it is no longer needed. --------- Co-authored-by: Xinyuan Lin <xinyual3@uci.edu>
Purpose
This PR sets user system to be enabled by default in the configuration. Currently, this flag is by default set to be disabled (a.k.a. the non-user mode). As no one is using the non-user mode and we are requiring all the developers to enable the user system, we have decided to abandon the non-user mode.
Challenge & Design
The major blocker of setting the flag to be enabled by default is two e2e test suites that rely on the non-user mode. These two test suites execute a workflow in the Amber engine in each of their test cases. Enabling the user mode would require texera_db in the test environment, as in the user-system mode, the execution of a workflow requires an
eid(and subsequently avid,wid, anduid) intexera_db.We could use
MockTexeraDB, which is currently used by many unit tests.MockTexeraDBcreates an embedded postgres instance per test suite, and the embedded db is destroyed at the end of each such test suite.However, a complexity of the two e2e test cases is they both access a singleton resource
WorkflowExecutionsResource, which caches the DSL context fromSqlServer(i.e., it only gets evaluated once per JVM):In fact, most of the singleton resources in our current codebase cache the
DSLContext/ Dao, as theDSLContextnever gets updated during the real Texera environment (i.e., the realtexera_db's address never changes).In the test environment, however, when working with
MockTexeraDB, that assumption does not hold, as each instance ofMockTexeraDBhas a different address, and gets destroyed before other test suite runs. Since all the test suites are executed in the same JVM during CI run, usingMockTexeraDBwould cause the 2nd of the two e2e test cases to fail because it still uses the DSL context from the 1st test suite'sMockTexeraDB.The diagrams below show what happens when using the embedded
MockTexeraDBto run two e2e test suites that both need to access the same singleton resource during their execution.The 1st test suite creates an embedded DB (

DB1) and lets the singletonSqlServerobject set itsDSLContextto point toDB1. When the test cases first accessWorkflowExecutionsResource(WER), WER grabs theDSLContextfromSqlServerand caches it.WERthen queriesDB1for all the test cases of test suite 1. When test suite 1 finishes,DB1gets destroyed.Later, In the same JVM, when test suite 2 starts, it also creates its own embedded DB (

DB2) and letsSqlServerpoint toDB2. However, as theDSLContextinWERis cached, it does not get updated when the test cases accessWER, soWERstill points toDB1, which is already destroyed, and causes failures.To solve this problem, we could either:
MockTexeraDB.We choose the 2nd design, as these two are e2e tests which should emulate production behavior with a real database. To avoid polluting the developer's local
texera_db, we use a separate test database with the same schema.Changes
user-systo be enabled by default.texera_db_for_test_casesspecifically for test cases and CIs.texera_ddl.sqlis updated to allow creating the database with a name other thantexera_db(and still defaults totexera_db), and CIs will automatically createtexera_db_for_test_caseswith the same schema astexera_db.DataProcessingSpecandPauseSpecto usetexera_db_for_test_cases. The two test suites now populate and cleanup this database during their run.MockTexeraDBis updated to incorporate the changes to the DDL script.SqlServeris also updated with aclearInstancelogic so that other unit tests that useMockTexeraDBcan clear their instance inSqlServerproperly so that they do not interfere with the two e2e tests.Next Step
Remove the
user-sys'senabledflag and itsif-elsehandling logic completely.