Thanks for taking time to review the patch Mike. My comments are in-line.
Mike Matrigali (JIRA) wrote:
[ http://issues.apache.org/jira/browse/DERBY-1156?page=all ]
Mike Matrigali updated DERBY-1156:
----------------------------------
Here are my comments on review of the reencrypt_4.diff, also I am
running storeall but it has not finished yet.:
minor typo's:
XactFactory.java line 835 - trasaction --> transactions
TransactionTable.java: add comment that hasPreparedXact(boolean recovered) is
also MT unsafe.
I will fix the comments.
Is there a test for readonly db?
There is a store/TurnsReadOnly.java test , but that is not included
in the regresssion test suites, because cleanup will fail. May be
there are some other tests or may be not :-)
It is a little wierd to throw an exception from the ReadOnly impl of
checkVersion(), from the name of the routine. I understand what the
comment is saying. It seems unexpected for this routine to not return
ok for a "current" db.
I think returning Ok (true) is not a right thing to do unless I
really check the versions by reading the versions from the control
files ..etc.
Current usage of this function is to make sure database is at the
right version before doing any writes that will break the soft-upgrade.
If some one in the future implements a read-only feature that requires
a version check , they can implement this method. Not my itch at the
moment :-)
Other choices are, throw the error :
1) StandardException.newException(SQLState.DATABASE_READ_ONLY);
instead of unimplmented.
2) Make sure checkVersion(...) is not called by doing the readOnly db
check in calling method.
I was planning to do the option 2 later. If you think option 1 is
better I am ok with it too.
stuff for now or later:
o may be interesting to add following to your tests on XA
o have a global xact in the log that is aborted during recovery (ie.
not yet prepared).
o have a global xact in the log that is prepared and committed.
I don't think there are code issues with these paths, mostly just easy
cases to add and will verify future code doesn't break it.
o add test for readonly db and reencrypt.
o add test for upgrade fail. Is there an existing framework for soft
upgrade testing in 10.2?
Thanks, those are good tests cases , I will work on them as a
different patch.
-suresh
