[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[
List Home]
|
RE: [stellation-res] Database Package Audit.
|
> >-----Original Message-----
> >From: stellation-res-admin@xxxxxxxxxxxxxxx
> >[mailto:stellation-res-admin@xxxxxxxxxxxxxxx]On Behalf Of Mark C.
> >Chu-Carroll
> >Sent: August 31, 2002 7:57 AM
> >To: stellation-res@xxxxxxxxxxxxxxx
> >Subject: RE: [stellation-res] Database Package Audit.
> >
> >
> >On Sat, 2002-08-31 at 15:10, Jonathan Gossage wrote:
> >>I will be sending a separate document discussing extensions to the
> >>database support needed by MySql as well as some suggestions for using
> >>database batch facilities and other performance enhancing methods that
> >>may be database specific.
> >
> >Batching is what I'm trying to do next. There's preliminary support
> >for it in the new database code, and I'm going to be modifying the code
> >to use batch where appropriate. We never did it before because postgres
> >doesn't really support it. Now we're adding support for quite a few
> >databases, and postgres is the only one that doesn't do JDBC batch.
> >
You might want to consider a couple of scenarios here. The first would be
"manual" batching where the batching decisions are made in the code the
generates the SQL statements. The second would be to push both batching and
SQL statement generation lower down into database specific code and simply
pass in the collection of values that are to be inserted or updated along
with a template for the SQL statement. The routine would then take care of
batching and generation of the SQL "under the hood". This approach is
particularly attractive for MySQL since it has a special optimization for
INSERT which is considerably more efficient that simply batching INSERT
statements. MySQL allows INSERT statements with multiple VALUE sets, thus
allowing multiple rows to be inserted in a single INSERT statement. This is
the recommended optimization for bulk insertions in a table in MySQL.
> >
> >> > >Third: about transactions. The intention is that everything
> >always be in
> >> > >an explicitly committed transaction. You are absolutely correct that
> >> > >if that's the way we want it to work, it should trigger an error
> >> > >when you don't work within a transaction. I will fix that before the
> >> > >next checkin.
> >>
> >> This should be easy to implement since there can be only one
> >outstanding
> >> transaction at a time per database access point and a
> >transaction active
> >> flag can be stored there.
> >
> >Yes. It's definitely easy to implement.
> >
> >
> >> Have you considered starting to use Java 1.4 so as to get access to the
> >> assert
> >> feature. It may be the way to go for such situations. It at
> >least provides a
> >> simple minded simulation of Design By Contract.
> >
> >Yes, we have considered it. The main reason that we've postponed doing
> >it thus far is that the performance of Stellation is dramatically better
> >when using the IBM JDK, and IBM has not publicly released their version
> >of 1.4. (I just got the IBM internal pre-release of 1.4 last week. I
> >don't know when it will be available to non-IBM folks.) Our policy so
> >far has been to avoid using any tools that our outside contributors
> >can't use. I don't want to give up the performance of the IBM JIT, but I
> >also don't want to be doing our work on a version of the JVM that
> >outside users can't run.
> >
> >
> >> > >Fourth: I did not realize that the default value for long
> >string would
> >> > >cause on error on MySQL. I don't have MySQL manuals handy
> >at the moment:
> >> > >what's the problem with it? Is it that the size is too long? Or is
> >> > >it something more subtle?
> >>
> >> The problem is that in MySql all strings > 255 characters are
> >defined as one
> >> of fouur variants of TEXT and have no length qualifier. You can have
> >> TINYTEXT < 2^8 ch,
> >> TEXT < 2^16 ch
> >> MEDIUMTEXT < 2^24 ch
> >> LONGTEXT < 2^32 ch
> >>
> >> CHAR and VARCHAR types are always <= 255 characters.
> >
> >Ah... OK. That's odd. But lots of stuff about MySQL is odd compared
> >to other DBs.
> >
> >
> >
> >> > >
> >> > >
> >> > >The rest doesn't need any comment: you've found some real, serious
> >> > >problems, and we will fix them, ASAP.
> >> > >
> >> > > -Mark
> >> > >
> >> > >
> >> > >On Sat, 2002-08-31 at 12:56, Jonathan Gossage wrote:
> >> > >> Attached is a text file that contains a file by file
> >> > >discussion of all the
> >> > >> points that I noticed in reviewing the database package
> >> > >classes. Enjoy :-)
> >> > >>
> >> > >> Regards
> >> > >>
> >> > >> Jonathan Gossage
> >> > >>
> >> > >> Personal Email
> >> > >> jgossage@xxxxxxxx
> >> > >>
> >> > >> Business Email
> >> > >> jonathan@xxxxxxxxxxxxxx
> >> > >>
> >> > >> ----
> >> > >>
> >> > >
> >> > >> This document contains a file by file audit of the new
> >> > >database package.
> >> > >>
> >> > >>
> >> > >> org.eclipse.stellation.repos.database.AbstractDatabase
> >> > >>
> >> > >> Javadoc for connectToDatabase
> >> > >>
> >> > >> The param "options" is a good example of a method argument
> >> > >whose content cannot be determined from the name and type. In
> >> > >this particular case we have to follow the reference chain back
> >> > >through AbstractBBAccessPoint, where we discover that it is an
> >> > >argument to the constructor named "location". Moving on backward
> >> > >we find in the method workspace.Configure.database that this
> >> > >argument comes from an option named "location" in a set of
> >> > >options. We also find here that these options are stored in a
> >> > >workspace.Context object which is passed in when the
> >> > >workspace.Configure object is constructed. Again, moving back we
> >> > >can discover that the workspace.Configure object is constructed
> >> > >in the workspace.Svc.command method which receives the context
> >> > >as a parameter. Moving back yet once more we find that
> >> > >workspace.Svc.command is invoked from workspace.Svc.run where we
> >> > >finally discover where the workspace.Context object is created.
> >> > >We also discover that the options are embedded in the field
> >> > >"programArguments which is passed to the workspace.Svc.run
> >> > >method. Moving back one final step we discover that the options
> >> > >came from the arguments passed to main on the command line.
> >> > >>
> >> > >> Having got here we at least know where this location option
> >> > >came from but we still do not know what it looks like. It
> >> > >appears we need to go to external documentation to discover this
> >> > >information. The point of this tedious litany is that a
> >> > >substantial amount of work is needed to discover the rules
> >> > >governing the content of the "location" argument. This work can
> >> > >be greatly reduced by following one simple convention.
> >> > >>
> >> > >> Use @see with an external hyperlink to link to the section in
> >> > >the document that describes the contents of the argument or use
> >> > >an internal link if the contents of the argument are defined
> >> > >internally within the Javadoc..
> >> > >>
> >> > >> I promise I will not be so long winded in future but following
> >> > >this convention will save substantial amounts of time,
> >> > >particularly for newcomers to the code base. The only problem
> >> > >could come when major documentation restructuring breaks the
> >> > >link but a Javadoc audit as part of each major release should
> >> > >deal with this problem.
> >> > >>
> >> > >>
> >> > >> org.eclipse.stellation.repos.database.AbstractDBAccessPoint
> >> > >>
> >> > >> 1. The Javadoc for the constructor describes the previous
> >> > >implementation of the constructor. The argument list has
> >> > >changed. Again need a hyperlink for the argument "location".
> >> > >>
> >> > >> 2. The constructor uses a hard coded string to determine the
> >> > >database type. These strings are scattered throughout the code
> >> > >base in several modules. Should the database names not be stored
> >> > >in String constants so they can be used consistently
> >wherever needed.
> >> > >>
> >> > >> 3. Also some thought needs to be given to whether knowledge of
> >> > >the specific database in use should be so widely scattered.
> >> > >Ideally all other code should need is an access point. At the
> >> > >moment there is database sensitive code in the following
> >> > >non-database modules:
> >> > >>
> >> > >> org.eclipse.stellation.repos.LocalHandle
> >> > >> org.eclipse.stellation.workspace.Location
> >> > >> org.eclipse.stellation.scm.model.ScmAccessSpec
> >> > >> org.eclipse.stellation.scm.model.ScmEnvirons
> >> > >> org.eclipse.stellation.repos.artifact.TextArtifact
> >> > >>
> >> > >> 4. The Javadoc for the createRepository method needs to be
> >> > >updated. The parameter list has changed. Also there should be a
> >> > >hyperlink to a documentation section that discusses the contents
> >> > >of the JDOM document. It is very difficult to keep documentation
> >> > >in sync. if it is duplicated. Since most of the tables are
> >> > >created here, a hyperlink to the document that describes the
> >> > >database tables should be included here.
> >> > >>
> >> > >> 5. The "createRepository" method contains a startTransaction
> >> > >call but no endTransaction call. Unless special circumstances
> >> > >hold, it is much safer from a code maintenance viewpoint to have
> >> > >both the start and end of a transaction in the same method. When
> >> > >this is not possible, it should be clearly documented at the
> >> > >point where the startTransaction call is made where the
> >> > >endTransaction call will be made.
> >> > >>
> >> > >> 6. Do you not require Javadoc for getter methods? There is
> >> > >none for "getRepositoryTag".
> >> > >>
> >> > >> 7. "getAuthenticator" would benefit from a hyperlink to the
> >> > >point where authentication methods are discussed in the
> >documentation.
> >> > >>
> >> > >> 8. "instantiateAgents" is logging exceptions to stderr. Should
> >> > >it not be using Log4J?
> >> > >>
> >> > >> 9. "getDatabase" needs a Javadoc entry.
> >> > >>
> >> > >> 10. "supportsDBType" needs a Javadoc entry.
> >> > >>
> >> > >>
> >> > >> org.eclipse.stellation.repos.database.BasicDatabase
> >> > >>
> >> > >> 1. The implementation of "getLongStringType" will generate
> >> > >incorrect code for some database systems unless overridden. For
> >> > >example it could generate VARCHAR(4000) which is illegal in
> >> > >MySql. My feeling is that BasicDatabase should only provide
> >> > >default implementations for types that can be guaranteed to be
> >> > >usable, even if not optimal, on all database systems
> >> > >Accordingly, I would provide no implementation in BasicDatabase
> >> > >for "getStringType" and "getLongStringType".
> >> > >>
> >> > >> 2. Javadoc entries should be supplied for protected methods
> >> > >since these are used in derived classesand are of interest to
> >> > >developers. The following methods need Javadoc entries:
> >> > >> renderField
> >> > >> renderPrimaryKey
> >> > >> renderForeignKey
> >> > >> getCreateTableHeader
> >> > >> getCreateTableTrailer
> >> > >>
> >> > >> 3. You may want to consider setting auto commit to true in
> >> > >endTransaction. This would provide a safety net that would
> >> > >ensure the integrity of any SQL statements that were
> >> > >inadvertantly not included in a transaction. Alternatively if
> >> > >you want a policy that forces all SQL statements to be run in
> >> > >transactions, you need to add a mechanism to check for this and
> >> > >throw an exception if the contract is violated.
> >> > >>
> >> > >> 4. So that I understand, why do you use protected fields
> >> > >rather than providing protected accessor functions for derived
> >> > >classes. In general it is best to encapsulate data as fully
> >as possible.
> >> > >>
> >> > >>
> >> > >> org.eclipse.stellation.repos.database.DB2Database
> >> > >>
> >> > >> 1. The error text in the exception messages refers to Postgres
> >> > >rather than DB2.
> >> > >>
> >> > >>
> >> > >> org.eclipse.stellation.repos.database.Table
> >> > >>
> >> > >> 1. All methods need Javadoc.
> >> > >>
> >> > >>
> >> > >> org.eclipse.stellation.repos.database.TemplateExpander
> >> > >>
> >> > >>
> >> > >--
> >> > >Mark Craig Chu-Carroll, IBM T.J. Watson Research Center
> >> > >*** The Stellation project: Advanced SCM for Collaboration
> >> > >*** http://www.eclipse.org/stellation
> >> > >*** Work Email: mcc@xxxxxxxxxxxxxx ------- Personal Email:
> >> > >markcc@xxxxxxxxxxx
> >> > >
> >> > >
> >> > >_______________________________________________
> >> > >stellation-res mailing list
> >> > >stellation-res@xxxxxxxxxxxxxxx
> >> > >http://dev.eclipse.org/mailman/listinfo/stellation-res
> >>
> >> _______________________________________________
> >> stellation-res mailing list
> >> stellation-res@xxxxxxxxxxxxxxx
> >> http://dev.eclipse.org/mailman/listinfo/stellation-res
> >>
> >>
> >--
> >Mark Craig Chu-Carroll, IBM T.J. Watson Research Center
> >*** The Stellation project: Advanced SCM for Collaboration
> >*** http://www.eclipse.org/stellation
> >*** Work Email: mcc@xxxxxxxxxxxxxx ------- Personal Email:
> >markcc@xxxxxxxxxxx
> >
> >
> >_______________________________________________
> >stellation-res mailing list
> >stellation-res@xxxxxxxxxxxxxxx
> >http://dev.eclipse.org/mailman/listinfo/stellation-res
