You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by "phet (via GitHub)" <gi...@apache.org> on 2024/05/09 01:31:58 UTC
[PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
phet opened a new pull request, #3948:
URL: https://github.com/apache/gobblin/pull/3948
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [ ] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-2065
### Description
- [ ] Here are some details about my PR, including screenshots (if applicable):
We continue to face the "Too many connections" error while running `gobblin-service` tests; see, e.g. - https://github.com/apache/gobblin/pull/3944
this shows up as
```
java.io.IOException: Failure creation table TestDagStateStore
...
Caused by: java.sql.SQLNonTransientConnectionException: Too many connections
...
```
Adjusting the server variable `max_connections` (in [TestMetastoreDatabaseServer.java](https://github.com/apache/gobblin/compare/master...phet:gobblin:fix_gobblin-service_tests_too_many_connections?expand=1#diff-3a1f64166bc336495b2d153fe1f8bc0fdc9dd28450cb8de1d32578c2f2727517)) to `501` has fixed local execution.
While debugging, I realized not all of the `@AfterClass` annotations use `(alwaysRun = true)`, so following on to the recent mods in https://github.com/apache/gobblin/pull/3946
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
updated tests herein
### Commits
- [ ] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594864769
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/ReevaluateDagProcTest.java:
##########
@@ -61,18 +64,36 @@ public class ReevaluateDagProcTest {
private final long flowExecutionId = System.currentTimeMillis();
private final String flowGroup = "fg";
- void mockDMSS(DagManagementStateStore dagManagementStateStore) throws IOException, SpecNotFoundException {
+ private ITestMetastoreDatabase testMetastoreDatabase;
+ private DagManagementStateStore dagManagementStateStore;
+
+ @BeforeClass
+ public void setUpClass() throws Exception {
+ this.testMetastoreDatabase = TestMetastoreDatabaseFactory.get();
+ }
+
+ @BeforeMethod
+ public void setUp() throws Exception {
+ this.dagManagementStateStore = spy(MostlyMySqlDagManagementStateStoreTest.getDummyDMSS(this.testMetastoreDatabase));
+ mockDMSSCommonBehavior(dagManagementStateStore);
+ }
+
+ private void mockDMSSCommonBehavior(DagManagementStateStore dagManagementStateStore) throws IOException, SpecNotFoundException {
doReturn(FlowSpec.builder().build()).when(dagManagementStateStore).getFlowSpec(any());
doNothing().when(dagManagementStateStore).tryAcquireQuota(any());
doNothing().when(dagManagementStateStore).addDagNodeState(any(), any());
+ doNothing().when(dagManagementStateStore).deleteDagNodeState(any(), any());
doReturn(true).when(dagManagementStateStore).releaseQuota(any());
}
+ @AfterClass(alwaysRun = true)
+ public void tearDownClass() throws Exception {
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ this.testMetastoreDatabase.close();
+ }
+
@Test
public void testOneNextJobToRun() throws Exception {
- ITestMetastoreDatabase testMetastoreDatabase = TestMetastoreDatabaseFactory.get();
- DagManagementStateStore dagManagementStateStore = spy(MostlyMySqlDagManagementStateStoreTest.getDummyDMSS(testMetastoreDatabase));
- mockDMSS(dagManagementStateStore);
Review Comment:
while each test uses divergent mocking, they can still initialize the bulk of the spy setup in a `@BeforeMethod`, so resituated it there
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 merged PR #3948:
URL: https://github.com/apache/gobblin/pull/3948
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594863028
##########
gobblin-metastore/src/test/java/org/apache/gobblin/metastore/testing/TestMetastoreDatabaseServer.java:
##########
@@ -96,6 +96,8 @@ class TestMetastoreDatabaseServer implements Closeable {
.withPort(this.dbPort)
.withUser(this.dbUserName, this.dbUserPassword)
.withServerVariable("explicit_defaults_for_timestamp", "off")
+ // default `max_connections` is apparently 151 - see: https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_max_connections
+ .withServerVariable("max_connections", "501")
Review Comment:
this seemed to be the deciding factor for successful local execution. most of the other changes are more cosmetic and for uniformity
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594883908
##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_store/MysqlBaseSpecStoreTest.java:
##########
@@ -48,23 +48,23 @@ public class MysqlBaseSpecStoreTest {
private static final String PASSWORD = "testPassword";
private static final String TABLE = "base_spec_store";
+ private ITestMetastoreDatabase testDb;
Review Comment:
although this is more rare w/ tests (however we DO have a few derived test classes), but anyway a `static` resource that needs closing doesn't really play well w/ subclassing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594863569
##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/cli/JobStateStoreCliTest.java:
##########
@@ -132,6 +132,14 @@ public void setUp() throws Exception {
jobState);
}
+ @AfterClass(alwaysRun = true)
+ public void tearDown() throws Exception {
Review Comment:
moving `tearDown` always to directly follow `setUp`, whenever it didn't
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594884624
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceHATest.java:
##########
@@ -255,6 +251,8 @@ public void cleanUp() throws Exception {
cleanUpDir(COMMON_SPEC_STORE_PARENT_DIR);
mysql.stop();
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ testMetastoreDatabase.close();
Review Comment:
true, but the canonical rule is to close in reverse order of opening. because, this was the first one to be opened, it should be the last closed
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceHATest.java:
##########
@@ -255,6 +251,8 @@ public void cleanUp() throws Exception {
cleanUpDir(COMMON_SPEC_STORE_PARENT_DIR);
mysql.stop();
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ testMetastoreDatabase.close();
Review Comment:
true, but the canonical rule is to close in reverse order of opening. as this was the first one to be opened, it should be the last closed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594872228
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceHATest.java:
##########
@@ -255,6 +251,8 @@ public void cleanUp() throws Exception {
cleanUpDir(COMMON_SPEC_STORE_PARENT_DIR);
mysql.stop();
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ testMetastoreDatabase.close();
Review Comment:
what if line 253 throws the exception and this statement is never reached
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594916582
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceHATest.java:
##########
@@ -255,6 +251,8 @@ public void cleanUp() throws Exception {
cleanUpDir(COMMON_SPEC_STORE_PARENT_DIR);
mysql.stop();
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ testMetastoreDatabase.close();
Review Comment:
aah, sorry, I misunderstood the gist of your Q earlier and now see what you were getting at. just made the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "arjun4084346 (via GitHub)" <gi...@apache.org>.
arjun4084346 commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594873737
##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_store/MysqlBaseSpecStoreTest.java:
##########
@@ -48,23 +48,23 @@ public class MysqlBaseSpecStoreTest {
private static final String PASSWORD = "testPassword";
private static final String TABLE = "base_spec_store";
+ private ITestMetastoreDatabase testDb;
Review Comment:
why cannot it be static always
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594916582
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/core/GobblinServiceHATest.java:
##########
@@ -255,6 +251,8 @@ public void cleanUp() throws Exception {
cleanUpDir(COMMON_SPEC_STORE_PARENT_DIR);
mysql.stop();
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ testMetastoreDatabase.close();
Review Comment:
aah, I misunderstood the gist of your Q earlier and now see what you were getting at. just made the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GOBBLIN-2065] Increase `TestMetastoreDatabaseServer` concurrent cnxns; plus make closing `ITestMetastoreDatabase` `alwaysRun = true` and doc reason [gobblin]
Posted by "phet (via GitHub)" <gi...@apache.org>.
phet commented on code in PR #3948:
URL: https://github.com/apache/gobblin/pull/3948#discussion_r1594862828
##########
gobblin-data-management/src/test/java/org/apache/gobblin/data/management/retention/CleanableMysqlDatasetStoreDatasetTest.java:
##########
@@ -112,6 +112,7 @@ public void setUp() throws Exception {
@AfterClass(alwaysRun = true)
public void tearDown() throws Exception {
if (testMetastoreDatabase != null) {
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
Review Comment:
added doc to alert future maintainers to the reason
##########
gobblin-metastore/src/test/java/org/apache/gobblin/metastore/testing/TestMetastoreDatabaseServer.java:
##########
@@ -96,6 +96,8 @@ class TestMetastoreDatabaseServer implements Closeable {
.withPort(this.dbPort)
.withUser(this.dbUserName, this.dbUserPassword)
.withServerVariable("explicit_defaults_for_timestamp", "off")
+ // default `max_connections` is apparently 151 - see: https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_max_connections
+ .withServerVariable("max_connections", "501")
Review Comment:
this seemed to be the deciding factor for successful local execution
##########
gobblin-service/src/test/java/org/apache/gobblin/service/modules/orchestration/proc/ReevaluateDagProcTest.java:
##########
@@ -61,18 +64,36 @@ public class ReevaluateDagProcTest {
private final long flowExecutionId = System.currentTimeMillis();
private final String flowGroup = "fg";
- void mockDMSS(DagManagementStateStore dagManagementStateStore) throws IOException, SpecNotFoundException {
+ private ITestMetastoreDatabase testMetastoreDatabase;
+ private DagManagementStateStore dagManagementStateStore;
+
+ @BeforeClass
+ public void setUpClass() throws Exception {
+ this.testMetastoreDatabase = TestMetastoreDatabaseFactory.get();
+ }
+
+ @BeforeMethod
+ public void setUp() throws Exception {
+ this.dagManagementStateStore = spy(MostlyMySqlDagManagementStateStoreTest.getDummyDMSS(this.testMetastoreDatabase));
+ mockDMSSCommonBehavior(dagManagementStateStore);
+ }
+
+ private void mockDMSSCommonBehavior(DagManagementStateStore dagManagementStateStore) throws IOException, SpecNotFoundException {
doReturn(FlowSpec.builder().build()).when(dagManagementStateStore).getFlowSpec(any());
doNothing().when(dagManagementStateStore).tryAcquireQuota(any());
doNothing().when(dagManagementStateStore).addDagNodeState(any(), any());
+ doNothing().when(dagManagementStateStore).deleteDagNodeState(any(), any());
doReturn(true).when(dagManagementStateStore).releaseQuota(any());
}
+ @AfterClass(alwaysRun = true)
+ public void tearDownClass() throws Exception {
+ // `.close()` to avoid (in the aggregate, across multiple suites) - java.sql.SQLNonTransientConnectionException: Too many connections
+ this.testMetastoreDatabase.close();
+ }
+
@Test
public void testOneNextJobToRun() throws Exception {
- ITestMetastoreDatabase testMetastoreDatabase = TestMetastoreDatabaseFactory.get();
- DagManagementStateStore dagManagementStateStore = spy(MostlyMySqlDagManagementStateStoreTest.getDummyDMSS(testMetastoreDatabase));
- mockDMSS(dagManagementStateStore);
Review Comment:
while each test uses divergent mocking, they can still initialize the bulk of the spy setup in a `@BeforeMethod`
##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/cli/JobStateStoreCliTest.java:
##########
@@ -132,6 +132,14 @@ public void setUp() throws Exception {
jobState);
}
+ @AfterClass(alwaysRun = true)
+ public void tearDown() throws Exception {
Review Comment:
moving `tearDown` whenever it didn't directly follow `setUp`
##########
gobblin-runtime/src/test/java/org/apache/gobblin/runtime/spec_store/MysqlBaseSpecStoreTest.java:
##########
@@ -48,23 +48,23 @@ public class MysqlBaseSpecStoreTest {
private static final String PASSWORD = "testPassword";
private static final String TABLE = "base_spec_store";
+ private ITestMetastoreDatabase testDb;
Review Comment:
mostly this DB is NOT `static`, but sometimes it was. for uniformity making it always instance-level
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org