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

Issue 1037 #1517

Merged
merged 7 commits into from Apr 26, 2016

Conversation

Projects
None yet
4 participants
@arienkock
Contributor

arienkock commented Apr 13, 2016

#1037 Simple fix to make it possible to instantiate an empty connection pool. For cases where you still want the webapp to load, e.g. you want a service to be able to start and show an error page when a DB isn't available.

Includes tests. Please ignore the changes to travis.yml

.travis.yml Outdated
after_success:
- bash .travis_after_success.sh
#after_success:
# - bash .travis_after_success.sh

This comment has been minimized.

@nickbabcock

nickbabcock Apr 13, 2016

Contributor

I'm assuming these lines are commented out by mistake, right? 😄

This comment has been minimized.

@arienkock

arienkock Apr 13, 2016

Contributor

Just to make CI work on my branch. As mentioned in the comments, changes to this file can be ignored.

This comment has been minimized.

@nickbabcock

nickbabcock Apr 13, 2016

Contributor

Ah yes, but if you do it for this pull request, it as no chance of being accidentally merged and saves us time. We're lazy too!

factory.setInitialSize(0);
ManagedDataSource dataSource = factory.build(metricRegistry, "test");
try {
dataSource.start();

This comment has been minimized.

@joschi

joschi Apr 13, 2016

Member

Surrounding the statement with try-catch doesn't gain anything. The test will fail anyway, if any uncaught exception occurs.

This comment has been minimized.

@nickbabcock

nickbabcock Apr 22, 2016

Contributor

@arienkock what about this?

@@ -107,6 +109,17 @@ public void testInlineUserPasswordConfiguration() throws Exception {
assertThat(ds.getUser()).isNull();
assertThat(ds.getPassword()).isNull();
}
// Test added for https://github.com/dropwizard/dropwizard/issues/1037

This comment has been minimized.

@joschi

joschi Apr 13, 2016

Member

Please remove that comment.

@Test
public void testInitialSizeZeroIsAllowed() throws Exception {
try {
DataSourceFactory ds = getDataSourceFactory("yaml/empty_initial_pool.yml");

This comment has been minimized.

@joschi

joschi Apr 13, 2016

Member

Surrounding the statement with try-catch doesn't gain anything. The test will fail anyway, if any uncaught exception occurs.

This comment has been minimized.

@arienkock

arienkock Apr 13, 2016

Contributor

Failing in the catch case makes it possible to distinguish between expected behavior (assertion exceptions) and unexpected problems (e.g. build env stuff)

This comment has been minimized.

@joschi

joschi Apr 13, 2016

Member

In general I'd agree, but that argument is kind of moot in a test consisting of a single statement (and one statement for assertions).

@coveralls

This comment has been minimized.

coveralls commented Apr 13, 2016

Coverage Status

Coverage remained the same at 80.9% when pulling 8f7eb58 on arienkock:issue-1037 into f0a2376 on dropwizard:master.

@arienkock

This comment has been minimized.

Contributor

arienkock commented Apr 19, 2016

In case it was lost in the exchange: the travis config on my branch was reverted as suggested.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Apr 21, 2016

This looks good. There are a few suggestions in the code review that should be applied.

Do you also have a link to documentation on this behavior for tomcat's jdbc pool for future reference?

@arienkock

This comment has been minimized.

Contributor

arienkock commented Apr 21, 2016

Sure. From the Javadoc of the ConnectionPool constructor: Instantiate a connection pool. This will create connections if initialSize is larger than 0.

Just pushed changes w.r.t. code review.

@coveralls

This comment has been minimized.

coveralls commented Apr 21, 2016

Coverage Status

Coverage increased (+0.02%) to 80.923% when pulling b001e25 on arienkock:issue-1037 into f0a2376 on dropwizard:master.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Apr 21, 2016

Did you miss one of the code review items?

@arienkock

This comment has been minimized.

Contributor

arienkock commented Apr 22, 2016

In the last commit I removed the try catch and code comment before the unit test.

@nickbabcock

This comment has been minimized.

Contributor

nickbabcock commented Apr 22, 2016

And just so I understand clearly, those that are using Dropwizard and JDBI -- the DBIHealthCheck, which needs a connection to the database, will fail, and this is the desired result. The application will still start, but will essentially be in an unhealthy state.

@arienkock

This comment has been minimized.

Contributor

arienkock commented Apr 26, 2016

Correct, the argument being that if the app can run without a DB (and be in an unhealthy state) why would it not be able to start. Useful in cases where you know a DB connection is of bad quality and you don't want a deployment to fail.

  • I just removed the other try-catch from the unit test
@coveralls

This comment has been minimized.

coveralls commented Apr 26, 2016

Coverage Status

Coverage increased (+0.02%) to 80.923% when pulling 5313d4c on arienkock:issue-1037 into f0a2376 on dropwizard:master.

@nickbabcock nickbabcock merged commit cd9104d into dropwizard:master Apr 26, 2016

nickbabcock added a commit that referenced this pull request Apr 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment