Merge lp://qastaging/~jaypipes/drizzle/publisher-read-manifest into lp://qastaging/~drizzle-trunk/drizzle/development

++ For exception handling :) Personally I don't like the use of std::runtime_error() since it accepts a string argument, which encourages building of strings as exception messages. This can itself lead to exceptions being throw and then you don't even get a proper message. Moving forward, we might want to create an exception type that uses a static sized buffer so there is less risk of string throwing exceptions.

Agreed on runtime_error and your concerns around it. Would be nice to finalize our discussions on exception handling and add some recommendations to our coding standards. I'm game to follow any advice.

-jay

On Wed, 28 Apr 2010 18:23:58 -0000, Eric Day <email address hidden> wrote:
> Review: Approve
> ++ For exception handling :) Personally I don't like the use of std::runtime_error() since it accepts a string argument, which encourages building of strings as exception messages. This can itself lead to exceptions being throw and then you don't even get a proper message. Moving forward, we might want to create an exception type that uses a static sized buffer so there is less risk of string throwing exceptions.

Very much agree on strings as error arguments.

In this case it seems to be just throwing a bool - i.e. some generic
failure.

we should probably catch other errors. I can think of ENOENT, ENOSPC,
EIO for three that should be handled gracefully.

--
Stewart Smith

Hi!

I would rather not see replication.proto move out of the kernel and off into the plugin. Basic reason why? If I start to develop a replication piece for Drizzle, I am going to want to use whatever is shipped with the kernel. If this piece is moved off I am likely to just develop my own.

Also? I think we should be pretty standard in our use of proto. Schema, Table, Row all have usage beyond just replication. If we start picking this stuff out we will likely end up finding ourselves with multiple.

On testing frameworks. We have one, Monty has one in his tree, and Eric has another. This is a lot of unit testing. Lets come up with a plan for one (and one that is used by many, not few).

Cheers,
   -Brian

On replication.proto:

As discussed before, the replication.proto contains messages that are specific to the Publisher and Subscriber *implementations* inside the transaction_log module, and are not relevant for other publishers and subscribers who may or may not use such manifest messages to keep communication in-sync between themselves. For instance, a Publisher and Subscriber plugin for RabbitMQ would use something entirely different. This is why the common messages are in /drizzled/message/transaction.proto and the implementation-dependent messages are in /plugin/transaction_log/replication.proto

On unittests:

You said you wanted tests of the replication system? I used the same method of testing that Eric did for the mysql_protocol module. The current test-run.pl cannot test the things that are being tested here. Please let me know how you want me to change things. Thanks.

On 05/07/2010 12:04 PM, Jay Pipes wrote:

> On unittests:
>
> You said you wanted tests of the replication system? I used the same
> method of testing that Eric did for the mysql_protocol module. The
> current test-run.pl cannot test the things that are being tested
> here. Please let me know how you want me to change things. Thanks.

My suggestion here is this:

Keep Jay's system and Eric's system for now. Test-run is in need of a
good overhauling anyway. We've got a summer of code student who is going
to be working on code-level unittesting. At that point, we should have
enough examples and data to work from to do a proper refactoring into
something we can actually use and that meets all of the needs we have.
(Consider this prototyping and requirements gathering)

I fear if we try to just quickly do something consistent right now we'll
wind up with something just as ass-tastic as test-run.

Monty

Hi!

The basics of one of the tests below is that you copy in a file and see what error message happens. test.pl can do this, we test this with dfe files already (and in the past have done this with MyISAM files as well).

I don't see why you need another test framework at this point to test these things.

I'd like to see more reasons why we aren't just using the existing framework before we just add additional ones.

Cheers,
   -Brian

Because I can't use the existing system to test the communication between publisher and subscriber. Since I can't use the existing system for that, I'll need another testing framework anyway.

I would also note that I am testing for the output of the error thrown in the publisher's constructor here. There is a bug in the server and main test-run.pl right now which prevents me from catching errors in the plugin startup/registration phase, and that's one more reason I can't use the existing test-run.pl:

https://bugs.launchpad.net/drizzle/+bug/570948

How about this. I would like to see tests in our current system where possible, which means I would like to see some tests on this code with the current system. Any additional verification you want to do is great, but I would like to see the basics in the current framework.

If there are bugs, not limitations, in the current system, we need to have them fixed.

Cheers,
   -Brian

OK, no prob. Could you advise how to start the server after copying a file from the /plugin/transaction_log/tests/inc/ directory? The copy needs to occur before the server startup, as the tests verify behaviour of corrupted manifest files that are already located in the datadir.

Thanks!

jay

Did you get the information I mentioned in IRC on how to do that?

Cheers,
  -Brian

Are you referring to your suggestion of using the broken_table_proto test? I had responded on IRC that that wouldn't work. I need to copy a file to the datadir *before* the server is started, not after, since the tests are verifying two things:

* A proper error message and disabling of the transaction_log module if a corrupted manifest file is present in the datadir when the server is started
* a proper startup and data_dictionary SELECT when a non-corrupted manifest file is present in the datadir on startup.

Cheers,
jay

Hi!

Have you seen how we copy a state of the server over before we start up? AKA how we copy in the opt files which represent the schemas that we use. Look at how schema_writer is called.

Cheers,
 -Brian

On May 18, 2010, at 12:15 PM, Jay Pipes wrote:

> Are you referring to your suggestion of using the broken_table_proto test? I had responded on IRC that that wouldn't work. I need to copy a file to the datadir *before* the server is started, not after, since the tests are verifying two things:
>
> * A proper error message and disabling of the transaction_log module if a corrupted manifest file is present in the datadir when the server is started
> * a proper startup and data_dictionary SELECT when a non-corrupted manifest file is present in the datadir on startup.
>
> Cheers,
> jay
> --
> https://code.launchpad.net/~jaypipes/drizzle/publisher-read-manifest/+merge/24255
> You are reviewing the proposed merge of lp:~jaypipes/drizzle/publisher-read-manifest into lp:drizzle.

> Have you seen how we copy a state of the server over before we start up? AKA
> how we copy in the opt files which represent the schemas that we use. Look at
> how schema_writer is called.

The only thing I could find which references schema_writer was in test-run.pl:

jpipes@serialcoder:~/repos/drizzle/trunk$ ack-grep "(schemawriter|schema_writer)"
tests/test-run.pl
154:our $exe_schemawriter;
1288:# Look for schema_writer
1290: $exe_schemawriter= mtr_exe_exists("$glob_basedir/drizzled/message/schema_writer",
1291: "$glob_builddir/drizzled/message/schema_writer");
1814: system("$exe_schemawriter mysql $data_dir/mysql/db.opt");
1817: system("$exe_schemawriter test $data_dir/test/db.opt");

The above is a hack in the setup_vardir() function in test-run.pl specifically for the schema_writer stuff. Are you saying that each and every plugin that needs to do something ike copy specific files before server startup should be modifying the test-run.pl script only for their needs? Seems like MyISAM-only tests all over again...

-jay