-
Notifications
You must be signed in to change notification settings - Fork 27
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
R3SOL-0 - remove hibernate from uniqueness #6378
R3SOL-0 - remove hibernate from uniqueness #6378
Conversation
Created a copy of `HoldingIdentity` named `UniquenessHoldingIdentity`.
…untime-os into nandor/R3SOL-372/extract-uniqueness
…remove-hibernate-from-uniqueness
Jenkins build for PR 6378 build 9 Build Successful: |
Building E2E Tests on PR-6378 Succeeded |
build e2e |
Should target |
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.
@nkovacsx can you review.
) : this( | ||
JPABackingStoreImpl( | ||
getEntityManagerFactory = { getEntityManagerFactory(virtualNodeInfoReadService, dbConnectionManager, jpaEntitiesRegistry, it) }, | ||
SqlBackingStoreImpl( |
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.
We should probably leave this one as the JpaBackingStoreImpl
and have another implementation of JPABackingStoreOsgiImpl
, or, renamed JPABackingStoreOsgiImpl
to BackingStoreImpl
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.
I'm somewhat leaning towards leaving the jpa implementation as the default implementation. We can then leverage the sql implementation as needed to remove the hibernate dep. I want to reduce the "risk" of this change and deal with it later on, if at all.
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.
We can do, but you'll end up having to maintain a lot more code (and tests).
It also means pulling the jpa backing store implementation in a separate module (or moving it back tocomponents/uniqueness/backing-store-impl
)
) | ||
init { | ||
// uncomment this to run the test against local Postgres | ||
System.setProperty("databaseType", "POSTGRES") |
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.
Does this mean this line should be commented out?
statement.setBytes(5, details.consumingId.bytes) | ||
|
||
statement.addBatch() | ||
println(statement) |
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.
Remove println.
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.
There are a few in this file.
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.
They are all in the test, which I though may be ok?
uniquenessSecureHashFactory | ||
) | ||
) | ||
connection.close() |
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.
Put the connection.close
only in the finally
?
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.
I copied the existing structure, but you're right, it doesn't need to be there.
I actually refactored it now to use use
on the connection which is a bit more idiomatic.
The try/finaly is still there for the metrics.
@@ -0,0 +1,87 @@ | |||
package net.corda.ledger.libs.uniqueness.backingstore.impl | |||
|
|||
class DefaultSqlQueryProvider : SqlQueryProvider { |
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.
I assume this only runs against postgres, not sure what the tests were doing beforehand.
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.
Correct. It could be made to work against the in-memory DB, but I haven't had time yet to do that.
The tests that are not working against the in-memory DB are skipped
|
New PR: #6380 |
Create new backing store that just uses SQL.
Targetting Nandor's branch.
Hibernate/JPA dependencies can be pulled out after verification.