db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Suresh Thalamati <suresh.thalam...@gmail.com>
Subject Re: [jira] Updated: (DERBY-1156) allow the encrypting of an existing unencrypted db and allow the re-encrypting of an existing encrypted db
Date Thu, 13 Jul 2006 18:51:45 GMT
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





Mime
View raw message