Skip to content
This repository has been archived by the owner on Jun 7, 2021. It is now read-only.

Integrate Agroal Datasource pool #1157 #1159

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

johnaohara
Copy link
Contributor

Integrate Agroal database connection pool to provide a fast, efficient database connection pool when a full jca compliant database connection pool is not required.

@thorntail-ci
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@thorntail-ci
Copy link
Contributor

Can one of the admins verify this patch?

}

/**
* Are transactions are managed by JTA.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the second "are".

return this;

}
public boolean isConnectable() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this attribute mean?


private Duration reapTimeout = ZERO;

private volatile Duration acquisitionTimeout = ZERO;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder why just 2 fields are volatile?

void noJDBCdriverClass(String className);

@LogMessage(level = Logger.Level.WARN)
@Message(id = 4 + OFFSET, value = "agroal datasource '%s' specified no JDBC driver, too many to choose from")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "no JDBC driver, too many to choose from" mean?

void implicitlyUsingDriver(String datasource, String driver);

@LogMessage(level = Logger.Level.WARN)
@Message(id = 6 + OFFSET, value = "agroal datasource '%s' specified requested tracing, but tracing is not available")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "specified"?

try {
jndi.bind(metaData.getJNDIName(), dataSource);
} catch (NamingException e) {
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...

}
if (this.metaData.isXa()) {
if (!XADataSource.class.isAssignableFrom(providerClass)) {
throw new RuntimeException("Driver is not an XA datasource and xa has been configured");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use the JBoss Logging error mechanism uniformly? Few lines above, we use AgroalMessages.MESSAGES.noJDBCdriverClass to throw an exception (at least I hope it throws an exception :-) ), but not here.

}
} else {
if (providerClass != null && !DataSource.class.isAssignableFrom(providerClass) && !Driver.class.isAssignableFrom(providerClass)) {
throw new RuntimeException("Driver is an XA datasource and xa has been configured");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message doesn't make a lot of sense.



private final AgroalDataSource delegate;
// private final TracedLocalManagedConnectionFactory mcf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unfinished?

/**
* @author kg6zvp
*/
public class JpaFraction { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this totally useless? Thorntail v4 has no concept of fractions.

@juangon
Copy link
Contributor

juangon commented Oct 29, 2018

@johnaohara please can you check the review comments about this PR?

Thanks!

@johnaohara
Copy link
Contributor Author

@juangon @Ladicek Only just seen the comments, missed the notifications. I will address the comments asap. Thanks

@sberyozkin sberyozkin added the 4.x label Nov 26, 2018
@kenfinnigan kenfinnigan merged commit b123354 into thorntail:4.x Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants