Remove storage cache layer (#324) #326
Conversation
| } | ||
|
|
||
| // cleanTestDB deletes all the entries in the database. Only use this with a test database | ||
| func cleanTestDB(db *sql.DB) { |
There was a problem hiding this comment.
Consider using a variant of the test specific new db logic in testonly/integration which creates a separate database for each test.
There was a problem hiding this comment.
It's not merged yet, added a TODO.
| } | ||
| } | ||
|
|
||
| func createTestDB(db *sql.DB) { |
There was a problem hiding this comment.
Consider using the temporary tree admin api in storage/mysql/log_admin.go
There was a problem hiding this comment.
As discussed offline, I'll add a TODO for now.
| } | ||
|
|
||
| // prepareTestTreeDB removes all database contents for the specified log id so tests run in a predictable environment. For obvious reasons this should only be allowed to run against test databases. This method panics if any of the deletions fails to make sure tests can't inadvertently succeed. | ||
| func prepareTestTreeDB(db *sql.DB, treeID int64, t *testing.T) { |
There was a problem hiding this comment.
Consider deleting the existing test database and creating a new one for test freshness.
| } | ||
|
|
||
| // prepareTestLogDB removes all database contents for the specified log id so tests run in a predictable environment. For obvious reasons this should only be allowed to run against test databases. This method panics if any of the deletions fails to make sure tests can't inadvertently succeed. | ||
| func prepareTestLogDB(db *sql.DB, logID logIDAndTest, t *testing.T) { |
There was a problem hiding this comment.
Consider using the tree admin api
There was a problem hiding this comment.
As discussed offline, I'll add a TODO for now.
The following changes have been made: * Open *sql.DB outside of storage (and reuse the same DB for all storage objects) * Test refactors (storage/mysql): * Moved DB-handling methods to storage_test (which contains TestMain) * Added a *sql.DB parameter to methods that access storage * Open DB connection only once for all tests * Cleaned DB at the beginning of every test The changes above render the storage caching layer obsolete.
codingllama
force-pushed
the
storage-cache
branch
from
b6c30ba to
0c581ca
Compare
|
These changes look great. |
|
Thanks, merging! |
|
If I run the log integration test there seem to be an awful lot of calls to (If I add a line to that function with |
|
GetLogStorage currently takes the LogID as an input parameter, forcing
calls to GetLogStorage to occur within each RPC.
So at the moment, this is expected, but not desirable behavior. Should be
fixed when #324 is addressed.
…On Tue, Jan 31, 2017 at 9:48 AM David Drysdale ***@***.***> wrote:
If I run the log integration test there seem to be an awful lot of calls
to defaultRegistry.GetLogStorage() -- is this expected?
(If I add a line to that function with log.Infof("Creating new storage
for log: %d", treeID) and turn on --alsologtostderr for the
trillian_log_server invocation in integration/log_integration_test.sh,
then running the test hits it ~850 times.)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#326 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMHTiN1M7hZfW8yCWvxJMs6BaHlefQ5ks5rXwNXgaJpZM4Ltuyi>
.
|
|
OK, thanks for checking. |
The following changes have been made:
The changes above render the storage caching layer obsolete.