[PATCH] Network Server DRDA XAMGR Level 7 Support (Server only)

[Kathey Marsden <km...@Sourcery.Org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Here are the Network Server changes for DRDA XAMGR Level 7 support.

Note: On the client side, there are some changes that will be required
to JCC.  These are not available the current DB2 Universal JDBC Driver.
 Because the changes will be  version dependent (notice this change
bumps the DRDA Maintenance version), the change needs to go into Derby
first. Then the client can be changed to send to send the proper
protocol based on the derby version.  I'll coordinate getting the
changes into the DB2 Universal JDBC Driver,  once these changes are in
Derby.

So this is just a necessary first step for XA support in Network Server.

Thanks

Kathey





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

iD8DBQFBtPjKG0h36bFmkocRApbPAJ90jHgFfmDJARcQ1NaJEKw8/lZfGACgwpil
pn4qhhyTWFvWkDTx752sMqA=
=FEiz
-----END PGP SIGNATURE-----

Re: [PATCH] Network Server DRDA XAMGR Level 7 Support (Server only)

[Kathey Marsden <km...@Sourcery.Org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kathey Marsden wrote:
>> Army wrote
>>>-- 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?
>
> 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.
>

Well this idea wasn't such a  swell idea.    I'd still have to do the
cast.
Maybe I'll leave it like it is for now unless someone has a better idea.

I've got the other changes made as described in earlier emails.    I'll
test tonight and send a patch with this item  unchanged unless anyone
has further input on this or any of the other issues.

Thanks

Kathey

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

iD8DBQFBt6p0G0h36bFmkocRAoIMAKDJQ8ZZO4cQ5c13I/DoRvaZ6+GjuwCgjnOg
6GAxDIBijsz4eZ1KZN8s/Fs=
=wK2Q
-----END PGP SIGNATURE-----

Re: [PATCH] Network Server DRDA XAMGR Level 7 Support (Server only)

[Kathey Marsden <km...@Sourcery.Org>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


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

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.

[snip]
>
> DRDAConnThread:
>
> Should there be a "try/catch" around the SYNCCTL case block in the
> "processCommands()" method?
[snip]
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.
[snip]
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.
NOTE
34189 LENGTH 32  <<<-----
34190 MINLVL 7
34191 OPTIONAL

[snip]
>
> -- 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.


[snip]
> -- 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?
>
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.

Thanks

Kathey


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

iD8DBQFBt1nrG0h36bFmkocRAkKjAJ4hvIYcbob5266A+iNlTslCtxrxKACeLOxW
60CoIbqaeg5o8plbUxbNP4A=
=IslG
-----END PGP SIGNATURE-----

Re: [PATCH] Network Server DRDA XAMGR Level 7 Support (Server only)

[Army <ar...@golux.com>]

Kathey Marsden wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> 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.

CodePoint.java:

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.

DRDAConnThread:

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.

DRDAStatement:

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.

DRDAXAProtocol:

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

Database.java

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

XADatabase.java

-- 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,
Army