Github either lets you do inline comments on the diff ("Files Changed" tab)
or on the entire commit ("Conversation"). One of the things review board
does right that's a bit frustrating in Github his that you can't group a
bunch of comments into a single review and post it all at once (and get
only one email!). Both tools have their drawbacks...On your particular comments: tests/kafkatest - Sort of redundant, but useful for being able to publish on PyPI, which will make services reusable by other projects. I actually hate this even about normal Python libraries too, where you need to have a top-level directory in the repository containing the project name. On the other hand, it actually keeps the organization fairly clean since it leaves space for docs, tests, and other directories, although those are less relevant here. (By the way, there are other issues with reusability that we may need to address eventually, such as the assumed layout of code in the workers; we probably need to get an initial version in and try reusing it in another project before we can really figure out everything that needs fixing for that.) multi-version - Great question, although I'd hope not to hold up an initial version only for this. Personally I think the right way to do this is to make each of the services support different versions (and for upgrade-style tests, they actually need to be able to change versions in some cases, i.e. rolling upgrades). To support this, we'll need to update the current build process (pre-test) to include grabbing the appropriate zips/jars, and the services will need to know about the layout of the trunk build vs. pre-built binaries. Then I think the tests just use the services but also specify versions (rather than relying on what I assume would be the default, which is the trunk build). If we want to test compatibility across many versions, then we might want to have a single test for compatibility between two versions, but make it parameterizable (which is another can of worms....) kafkatest/tests - Organizationally I think a single file will usually make sense, but you could also have an entire module if you needed enough code for the test that it made sense to spread it across multiple files. So far I think our experience has been that tests usually are pretty small because more of the code tends to end up in services or utilities. We can see if @granders agrees with that or not, he has now probably written more of these than I have. kafka/vagrant - This already existed as part of https://issues.apache.org/jira/browse/KAFKA-1173 These are updates that make it work well if you want to run the driver machine (the one running ducktape) on EC2, having it allocate more EC2 nodes to use as workers for the tests. The IP address you saw is EC2-specific, but the Vagrantfile also isolates those scripts to be used when using the vagrant-aws plugin. Other stuff like being Debian-specific can be generalized by extending the Vagrantfile and scripts, which I think would be great, but somebody needs to invest the time. There are loads of other improvements that could me made too -- for example, I'd love seeing Docker support, which would make running tests way cheaper/simpler. On Tue, Jun 23, 2015 at 7:56 PM, Gwen Shapira <gshap...@cloudera.com> wrote: > I'm unclear on the directory structure and few high level things (and > I can't figure out how to comment on more than a single line in > github): > > tests/kafkatest - isn't it redundant? do we expect any non-kafka tests? > > services/templates - those are basically all configuration files to be > used by the services, right? Do we expect a single set for the entire > system? or different templates for different tests? I'm checking > because "systemtests" had config files per test. > > any thoughts on how multi-version tests will work? will we have > service per version? > > kafkatest/tests - do we expect every test to be a single script? or > should we have subdirectories here, to start things in the right > direction? Maybe subdirectories for planned groups of tests? > > kafka/vagrant - is this intentionally not under tests? Is this at all > stand-alone? If so, maybe a separate Jira? This has a bunch of stuff > that I'm not sure we want in the project at all... specific IPs, > Vagrant install from debian packages, etc. > > Gwen > > > > > > > > > > On Tue, Jun 23, 2015 at 10:45 AM, Gwen Shapira <gshap...@cloudera.com> > wrote: > > Awesome, thanks :) > > > > On Tue, Jun 23, 2015 at 10:32 AM, Geoffrey Anderson <ge...@confluent.io> > wrote: > >> Hi Gwen, > >> > >> That is indeed the plan, and my understanding is that the merge script > >> Ismael has been working on helps committers with this step. > >> > >> I'm trying out the Github flow roughly as outlined here: > >> > http://mail-archives.apache.org/mod_mbox/kafka-dev/201504.mbox/%3ccad5tkzab-hkey-zcr8x4wtxawybxpojx62k1vbv+ycknuxq...@mail.gmail.com%3E > >> > >> Ismael's script is here: > https://issues.apache.org/jira/browse/KAFKA-2187 > >> > >> > >> Thanks, > >> > >> Geoff > >> > >> On Mon, Jun 22, 2015 at 9:59 PM, Gwen Shapira <gshap...@cloudera.com> > wrote: > >> > >>> Thanks, I indeed missed the original :) > >>> > >>> Is the plan to squash the commits and merge a pull request with single > >>> commit that matches the JIRA #? > >>> This will be more in line with how commits were organized until now > >>> and will make life much easier when cherry-picking. > >>> > >>> Gwen > >>> > >>> On Mon, Jun 22, 2015 at 1:58 PM, Geoffrey Anderson <ge...@confluent.io > > > >>> wrote: > >>> > Hi, > >>> > > >>> > I'm pinging the dev list regarding KAFKA-2276 (KIP-25 initial patch) > >>> again > >>> > since it sounds like at least one person I spoke with did not see the > >>> > initial pull request. > >>> > > >>> > Pull request: https://github.com/apache/kafka/pull/70/ > >>> > JIRA: https://issues.apache.org/jira/browse/KAFKA-2276 > >>> > > >>> > Thanks! > >>> > Geoff > >>> > > >>> > > >>> > On Tue, Jun 16, 2015 at 2:50 PM, granders <g...@git.apache.org> > wrote: > >>> > > >>> >> GitHub user granders opened a pull request: > >>> >> > >>> >> https://github.com/apache/kafka/pull/70 > >>> >> > >>> >> Kafka 2276 > >>> >> > >>> >> Initial patch for KIP-25 > >>> >> > >>> >> Note that to install ducktape, do *not* use pip to install > ducktape. > >>> >> Instead: > >>> >> > >>> >> ``` > >>> >> $ git clone g...@github.com:confluentinc/ducktape.git > >>> >> $ cd ducktape > >>> >> $ python setup.py install > >>> >> ``` > >>> >> > >>> >> > >>> >> You can merge this pull request into a Git repository by running: > >>> >> > >>> >> $ git pull https://github.com/confluentinc/kafka KAFKA-2276 > >>> >> > >>> >> Alternatively you can review and apply these changes as the patch > at: > >>> >> > >>> >> https://github.com/apache/kafka/pull/70.patch > >>> >> > >>> >> To close this pull request, make a commit to your master/trunk > branch > >>> >> with (at least) the following in the commit message: > >>> >> > >>> >> This closes #70 > >>> >> > >>> >> ---- > >>> >> commit 81e41562f3836e95e89e12f215c82b1b2d505381 > >>> >> Author: Liquan Pei <liquan...@gmail.com> > >>> >> Date: 2015-04-24T01:32:54Z > >>> >> > >>> >> Bootstrap Kafka system tests > >>> >> > >>> >> commit f1914c3ba9b52d0f8db3989c8b031127b42ac59e > >>> >> Author: Liquan Pei <liquan...@gmail.com> > >>> >> Date: 2015-04-24T01:33:44Z > >>> >> > >>> >> Merge pull request #2 from confluentinc/system_tests > >>> >> > >>> >> Bootstrap Kafka system tests > >>> >> > >>> >> commit a2789885806f98dcd1fd58edc9a10a30e4bd314c > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-05-26T22:21:23Z > >>> >> > >>> >> fixed typos > >>> >> > >>> >> commit 07cd1c66a952ee29fc3c8e85464acb43a6981b8a > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-05-26T22:22:14Z > >>> >> > >>> >> Added simple producer which prints status of produced messages > to > >>> >> stdout. > >>> >> > >>> >> commit da94b8cbe79e6634cc32fbe8f6deb25388923029 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-05-27T21:07:20Z > >>> >> > >>> >> Added number of messages option. > >>> >> > >>> >> commit 2777712b39a2d75027299fbb1b1008d463a82aab > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-05-27T22:35:06Z > >>> >> > >>> >> Added some metadata to producer output. > >>> >> > >>> >> commit 8b4b1f2aa9681632ef65aa92dfd3066cd7d62851 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-05-29T23:38:32Z > >>> >> > >>> >> Minor updates to VerboseProducer > >>> >> > >>> >> commit c0526fe44cea739519a0889ebe9ead01b406b365 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-01T02:27:15Z > >>> >> > >>> >> Updates per review comments. > >>> >> > >>> >> commit bc009f218e00241cbdd23931d01b52c442eef6b7 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-01T02:28:28Z > >>> >> > >>> >> Got rid of VerboseProducer in core (moved to clients) > >>> >> > >>> >> commit 475423bb642ac8f816e8080f891867a6362c17fa > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-01T04:05:09Z > >>> >> > >>> >> Convert class to string before adding to json object. > >>> >> > >>> >> commit 0a5de8e0590e3a8dce1a91769ad41497b5e07d17 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-02T22:46:52Z > >>> >> > >>> >> Fixed checkstyle errors. Changed name to VerifiableProducer. > Added > >>> >> synchronization for thread safety on println statements. > >>> >> > >>> >> commit 9100417ce0717a71c822c5a279fe7858bfe7a7ee > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-03T19:50:11Z > >>> >> > >>> >> Updated command-line options for VerifiableProducer. Extracted > >>> >> throughput logic to make it reusable. > >>> >> > >>> >> commit 1228eefc4e52b58c214b3ad45feab36a475d5a66 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-04T01:09:14Z > >>> >> > >>> >> Renamed throttler > >>> >> > >>> >> commit 6842ed1ffad62a84df67a0f0b6a651a6df085d12 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-04T01:12:11Z > >>> >> > >>> >> left out a file from last commit > >>> >> > >>> >> commit d586fb0eb63409807c02f280fae786cec55fb348 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-04T01:22:34Z > >>> >> > >>> >> Updated comments to reflect that throttler is not > message-specific > >>> >> > >>> >> commit a80a4282ba9a288edba7cdf409d31f01ebf3d458 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-04T20:47:21Z > >>> >> > >>> >> Added shell program for VerifiableProducer. > >>> >> > >>> >> commit 51a94fd6ece926bcdd864af353efcf4c4d1b8ad8 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-04T20:55:02Z > >>> >> > >>> >> Use argparse4j instead of joptsimple. ThroughputThrottler now > has > >>> more > >>> >> intuitive behavior when targetThroughput is 0. > >>> >> > >>> >> commit 632be12d2384bfd1ed3b057913dfd363cab71726 > >>> >> Author: Geoff <grand...@gmail.com> > >>> >> Date: 2015-06-04T22:22:44Z > >>> >> > >>> >> Merge pull request #3 from confluentinc/verbose-client > >>> >> > >>> >> Verbose client > >>> >> > >>> >> commit fc7c81c1f6cce497c19da34f7c452ee44800ab6d > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-11T01:01:39Z > >>> >> > >>> >> added setup.py > >>> >> > >>> >> commit 884b20e3a7ce7a94f22594782322e4366b51f7eb > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-11T01:02:11Z > >>> >> > >>> >> Moved a bunch of files to kafkatest directory > >>> >> > >>> >> commit 25a413d6ae3333938e9773eb2b20509760bab464 > >>> >> Author: Geoff <grand...@gmail.com> > >>> >> Date: 2015-06-11T20:29:21Z > >>> >> > >>> >> Update aws-example-Vagrantfile.local > >>> >> > >>> >> commit 96533c3718a9285d78393fb453b951592c72a490 > >>> >> Author: Geoff <grand...@gmail.com> > >>> >> Date: 2015-06-11T20:36:33Z > >>> >> > >>> >> Update aws-access-keys-commands > >>> >> > >>> >> commit e5edf031aeb99b9176a6ae8375963f2aedaaa6d7 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T00:27:49Z > >>> >> > >>> >> Updated example aws Vagrantfile.local > >>> >> > >>> >> commit 5af88fc1d9fc357c191a7c5fdbca60e37f42e9fc > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T00:28:19Z > >>> >> > >>> >> Updated README to include aws quickstart > >>> >> > >>> >> commit 4f476fec65e92ff5bf940dc4928e2fb64d424c0e > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T22:17:26Z > >>> >> > >>> >> Moved aws scripts to vagrant directory > >>> >> > >>> >> commit c60125cf4b983de958685cdcf10e7bab9813b119 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T22:18:59Z > >>> >> > >>> >> TestEndToEndLatency -> EndToEndLatency > >>> >> > >>> >> commit 7f7c3e0e68d9c3c50fb4b836b90887f39b43c466 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T22:20:25Z > >>> >> > >>> >> Updated setup.py for kafkatest > >>> >> > >>> >> commit 42dcdb1d66704bf512ddadc3020c493e74c5ba25 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T22:21:07Z > >>> >> > >>> >> Tweaked behavior of stop_node, clean_node to generally fail fast > >>> >> > >>> >> commit 3d738578d28cf732f5b1fc9d769b4bd487577082 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-12T22:24:30Z > >>> >> > >>> >> Merged downstream changes > >>> >> > >>> >> commit f469f84e6d827a580c26a62ef01f0e53f002c3e3 > >>> >> Author: Geoff Anderson <ge...@confluent.io> > >>> >> Date: 2015-06-15T20:13:17Z > >>> >> > >>> >> Tweaked readme, added example Vagrantfile.local > >>> >> > >>> >> ---- > >>> >> > >>> >> > >>> >> --- > >>> >> If your project is set up for it, you can reply to this email and > have > >>> your > >>> >> reply appear on GitHub as well. If your project does not have this > >>> feature > >>> >> enabled and wishes so, or if the feature is enabled but not working, > >>> please > >>> >> contact infrastructure at infrastruct...@apache.org or file a JIRA > >>> ticket > >>> >> with INFRA. > >>> >> --- > >>> >> > >>> > -- Thanks, Ewen
