Merge lp://staging/~diogobaeder/txstatsd/fix-host-lookup into lp://staging/txstatsd

A few comments:

1. I dislike class attributes with parameters that cannot be set through the constructor. How are you suppose to change LIMIT? By subclassing? By monkey patching?

2. Along the same lines, because you can't easily change LIMIT, the tests iterate calling write() up to 'LIMIT' times, which is useless. If LIMIT was configurable, you could set it to '1' in the tests and make them more efficient.

Fixed. Can you check again?

Em 26-12-2012 17:21, Sidnei da Silva escreveu:
> Review: Needs Fixing
>
> A few comments:
>
> 1. I dislike class attributes with parameters that cannot be set through the constructor. How are you suppose to change LIMIT? By subclassing? By monkey patching?
>
> 2. Along the same lines, because you can't easily change LIMIT, the tests iterate calling write() up to 'LIMIT' times, which is useless. If LIMIT was configurable, you could set it to '1' in the tests and make them more efficient.

You still haven't changed the test to use limit=1. :)

I changed it to use limit=2 instead of limit=1 because it's more
expressive to me that the limit is actually used/respected when queueing
the calls (if I use only 1, it's not clear that it respects the number
given as a limit or if it closes right after the first message sent to
the queue). This shouldn't have any noticeable impact on the test duration.

Would this be OK for you?

Em 26-12-2012 18:08, Sidnei da Silva escreveu:
> You still haven't changed the test to use limit=1. :)

Changed again to use limit=2 in the default instance for tests, instead
of in specific tests, as requested.

Em 26-12-2012 18:39, Diogo Baeder escreveu:
> I changed it to use limit=2 instead of limit=1 because it's more
> expressive to me that the limit is actually used/respected when queueing
> the calls (if I use only 1, it's not clear that it respects the number
> given as a limit or if it closes right after the first message sent to
> the queue). This shouldn't have any noticeable impact on the test duration.
>
> Would this be OK for you?
>
> Em 26-12-2012 18:08, Sidnei da Silva escreveu:
>> You still haven't changed the test to use limit=1. :)
>

I'm fairly confident that calling transport.connect() does not do what you want. That sets the transport to 'connected udp' which is not what we want. See: http://twistedmatrix.com/documents/11.0.0/core/howto/udp.html#auto2

Instead, you should pass host, port to the transport gateway and use them in transport.write(data, (host, port)).

Well, as far as I understood it, it doesn't create a "real connection",
only makes sure that all datagrams are delivered (when they are) to the
same host. As what we want is to send datagrams to the same host (as we
even resolve the host before sending the packets), this makes perfect
sense - and makes it cleaner to send messages, as we only have to pass
the data as an argument, instead of providing (host, port) for every
write() call.

Here, take a look at the paragraphs of the documentation that make it clear:
"A connected UDP socket is slightly different from a standard one - it
can only send and receive datagrams to/from a single address, but this
does not in any way imply a connection"
"Unlike a regular UDP protocol, we do not need to specify where to send
datagrams and are not told where they came from since they can only come
from the address to which the socket is 'connected'."

Em 27-12-2012 13:59, Sidnei da Silva escreveu:
> I'm fairly confident that calling transport.connect() does not do what you want. That sets the transport to 'connected udp' which is not what we want. See: http://twistedmatrix.com/documents/11.0.0/core/howto/udp.html#auto2
>
> Instead, you should pass host, port to the transport gateway and use them in transport.write(data, (host, port)).
>
>

Yes, but we tried that before and it didn't work as intended. We don't care about making sure that the datagrams are delivered so it's only adding overhead to it.

Ah, ok, good to know that, I'll avoid trying to fix the host in the
future, then. Now I changed it to use explicit (host, port) on every call.

Diff updated, can you take a look again?

Em 27-12-2012 15:24, Sidnei da Silva escreveu:
> Yes, but we tried that before and it didn't work as intended. We don't care about making sure that the datagrams are delivered so it's only adding overhead to it.

The attempt to merge lp:~diogobaeder/txstatsd/fix-host-lookup into lp:txstatsd failed. Below is the output from the failed tests.

txstatsd.tests.metrics.test_distinct
  TestDistinct
    test_all ... [OK]
  TestDistinctMetricReporter
    test_reports ... [OK]
  TestHash
    test_chi_square ... [SKIPPED]
    test_hash_chars ... [OK]
  TestPlugin
    test_factory ... [OK]
  TestZeros
    test_zeros ... [OK]
txstatsd.tests.metrics.test_histogrammetric
  TestHistogramReporterMetric
    test_histogram_histogram ... [OK]
    test_histogram_of_numbers_1_through_10000 ... [OK]
    test_histogram_with_zero_recorded_values ... [OK]
txstatsd.tests.metrics.test_metermetric
  TestDeriveMetricReporter
    test_fastpoll ... [OK]
    test_interface ... [OK]
txstatsd.tests.metrics.test_sli
  TestConditions
    test_above ... [OK]
    test_above_linear ... [OK]
    test_below ... [OK]
    test_below_linear ... [OK]
    test_between ... [OK]
  TestFactory
    test_configure ... [OK]
    test_configure_linear ... [OK]
  TestMetric
    test_clear ... [OK]
    test_count_all ... [OK]
    test_count_error ... [OK]
    test_count_threshold ... [OK]
    test_reports ... [OK]
  TestMetricLinear
    test_count_threshold ... [OK]
  TestParsing
    test_parse ... [OK]
txstatsd.tests.metrics.test_timermetric
  TestBlankTimerMetric
    test_count ... [OK]
    test_max ... [OK]
    test_mean ... [OK]
    test_min ... [OK]
    test_no_values ... [OK]
    test_percentiles ... [OK]
    test_rate ... [OK]
    test_std_dev ... [OK]...