db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kathey Marsden <kmars...@Sourcery.Org>
Subject Re: [PATCH] Network Server DRDA XAMGR Level 7 Support (Server only)
Date Wed, 08 Dec 2004 19:45:48 GMT
Hash: SHA1

Army wrote:
> My comments regarding the changes are as follow

Thanks Army for your thoughtful  review.
Comments below.  For anything I have omitted, you are absolutely right
and I will fix!

> CodePoint.java:
> Just curious: How did you find out that the value of TMLOCAL is
> X'10000000'?

I'm glad you mentioned this. This is a flag that JCC sends for local
transactions, but I couldn't find it in the specs either.  The spec just
says that the xid will be null (formatid == -1) and in fact the xid is
correct for local transactions, so I will just take that out. It doesn't
 seem to be adding any value.

> DRDAConnThread:
> Should there be a "try/catch" around the SYNCCTL case block in the
> "processCommands()" method?
These methods don't throw SQLExceptions like the other calls,
XAExceptions are already handled in DRDAXAProtocol.java so I think that
this is ok.    Let me know if you see a scenario that will cause trouble.

> -- 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.
Originally I had all the CodePoints for SYNCPTMGR level 5 throw
codePointNotSupported errors, but then I decided that since we send 0
for our SYNCPTMGR level we really didn't need any of this stuff and
invalidCodepoint would be ok.  Any thoughts?

> -- 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)...?
The funny thing is it is sent as a 32 bit number value (1-64) and the
javax.sql.Xid interface calls for it to be an integer so I went that way.

In the DDM manual under XID gtrid_length
gtrid_length INSTANCE_OF BINDR - Binary Number Field
34186 NOTE Contains a number between 1 and 64.
34187 The content and description of this number is
34188 not described by DDM.
34189 LENGTH 32  <<<-----
34190 MINLVL 7

> -- 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....?
I think I got overzealous when removing the SYNCPTMGR level 5 stuff and
took out RLSCONV too. I'll put it back in.

> -- 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?

These are all DRDA Protocol exceptions. XA protocol exceptions  that
would would be generated by the database server if you tried to an
illegal XA operation.  I think it is ok to ignore optional parameters if
they are not used by the specific operation rather than generating a
protocol exception.

> -- 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
I thought it better not to have another reference to  the XAResource in
the DRDAXAProtocol class. I think traditionally we have just tried to
keep the database as the only state in the DRDAConnectionThread since it
would add to the cleanup.   I think it brings up a good point though. I
should move the actual XA actions into the XADatabase class so that we
don't have to do this.   I'll handle it that way.


> -- 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...
No that's just for after the jcc attributes which we don't need to deal
with at this point.

> XADatabase.java
> -- In the "setDrdaID" method, you have the check:
>     if (getConnection() != null)
> but that check is NOT in the setPrepareIsolation() method.  Is that okay?
I think we should have a connection by then.

I'll get the changes in and resubmit soon.  Please let me know of any
additional feedback.



Version: GnuPG v1.2.5 (MingW32)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org


View raw message