db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Army <a...@golux.com>
Subject Re: [PATCH] Network Server DRDA XAMGR Level 7 Support (Server only)
Date Wed, 08 Dec 2004 18:31:38 GMT
Kathey Marsden wrote:
> Hash: SHA1
> Here are the Network Server changes for DRDA XAMGR Level 7 support.

[ snip ]

Hi Kathey,

My comments regarding the changes are as follow, based on what I read in the DDM manual for
XAMGR.  If you'd like more 
detail on any of these, or are not sure what I'm referring to, feel free to ask.


Just curious: How did you find out that the value of TMLOCAL is X'10000000'?  I searched my
versions of the DDM/DRDA 
specifications and I don't see that constant anywhere.  Mayby my specs are just out of date.


Should there be a "try/catch" around the SYNCCTL case block in the "processCommands()" method?
 It looks like most of 
the other case blocks have a try/catch and, in the event of an exception, call "clearDSSesBackToMark"
and then write an 
error indication.  Would it be useful to have that logic for SYNCCTL, too?  Except that, instead
of writing an SQLCARD, 
we'd write a SYNCCRD?  Maybe that's not really appropriate for XA processing, but I thought
I'd ask.


I think there are a handful of cases where we could use the "handleReflectionException" method
to reduce redundant code. 
  For example, in getResultSetHoldability and prepareStatementJDBC3.


-- I think we'll have to remove the "copyrightNotice" field from the class, per the new Derby
copyright rules.

-- It looks we need to add cases for REQ_COMMIT and MIGRATED to the parseSYNCCTL()  method
and throw 
codePointNotSupported errors for them, instead of throwing invalidCodePoint errors.

-- Should "gtridLen" and "bqualLen" be "int" or "long"?  They're treated as ints in the "writeXID"
and "parseXID" 
methods, but the spec says that they are long (64  bits)...?

-- Since the XID and XAFLAGS parameters are required, should there by code in parseSYNCCTL()
to verify that they have 
been provided and to throw a missingCodePoint error if they have not?

-- What's the correct behavior for optional parameters, such as RLSCONV?  Right now, the code
will throw 
invalidCodePoint errors--is that what we want?  It seems like it might be better to either
catch and ignore them (like 
we do with TIMEOUT), or else throw codePointNotSupported errors; not sure which is more appropriate....?

-- Similarly, should we be throwing XA protocol errors for cases that aren't allowed?  For
example, a "FORGET" parameter 
is never allowed for an XAMGR--is it okay to throw invalidCodePoint errors, or should we be
explicitly returning XA 
protocol errors?  What about for things like "this parameter can _only_ be specified if synctype
is X'09'"--should we 
catch those kinds of errors and return XA protocol exceptions, or not?

-- Is there a reason we only have logic to rollback XA transactions, even though we have logic
to commit both local 
transactions _and_ XA transactions?  Does it make sense to rollback a local transaction from
an XA connection?

-- Would it be worth it to call "getXAResource" a single time in the DRDAXAProtocol constructor,
and then reuse it, 
instead of having to repeatedly do the work of "((XADatabase) connThread.getDatabase()).getXAResource();"
 Or would that 
break something?


-- The two "p.put" calls at the beginning of the makeConnection() method seem redundant given
that the same two values 
are put into properties p right before the call to this method.  Would it be possible to remove
the duplicates, either 
from Database.makeConnection() or else from DRDAConnThread.getConnFromDatabase()?

-- Does the appendAttrString() method need to add a final ";" to the end of the string?  I
don't remember when that's 
required and when it's okay to leave it off...


-- I think we'll have to remove the "copyrightNotice" field from the class, per the new Derby
copyright rules.

-- In the "setDrdaID" method, you have the check:

	if (getConnection() != null)

but that check is NOT in the setPrepareIsolation() method.  Is that okay?


Again, feel free to ask if you have any questions about what I'm referring to with these comments...

Hope that helps,

View raw message