Checking if message timeout is shorter than buffering linger

[larry-cdn77]

Both global and topic configurations appear to implement this check for message timeout being shorter than queue buffering linger. This is a sensible check since one does want to end up with messages timing out before they can be batched into a message set.

The global configuration check also implements an auto-adjust of unmodified linger, ie correct linger if you left it at its default and set message timeout too high. That also makes sense since altering linger is acceptable (mainly a performance effect) while altering message timeout is much less so (risk undelivered messages).

I struggle to make either work, however: the checks or the auto adjust. Any combination of values leads to a valid producer being created withou error.

Upon inspection it appears to me that they should not be testing for linger modified using rd_kafka_topic_conf_is_modified as linger can not be a topic configuration. It is global.

            rd_kafka_topic_conf_is_modified(tconf, "linger.ms"))

I did not see #2974 and #3161 that brought this code add any tests, so wondered how it was intended to work. I am using a test like the below in 0004-conf.

/**
 * @brief Verify behaviour of checking that message timeout fits within
 *        configured linger
 */
static void do_message_timeout_linger_checks (void) {
        rd_kafka_conf_t *conf;
        rd_kafka_topic_conf_t *tconf;
        rd_kafka_t *rk;
        char errstr[512];
        int i;

        char values[7][3][40] = {
                {"-",     "-",      "default and L and M"},
                {"100",   "-",      "set L such that L<M"},
                {"-",     "300000", "set M such that L<M"},
                {"100",   "300000", "set L and M such that L<M"},
                {"500000", "-",     "!set L such that L>=M"},
                {"-",      "10",    "!set M such that L>=M"},
                {"500000", "10",    "!set L and M such that L>=M"}
        };

        for (i = 0; i < 7; i++) {
                conf = rd_kafka_conf_new();
		tconf = rd_kafka_topic_conf_new();

                if (values[i][0][0] != '-') {
                        test_conf_set(conf, "linger.ms",
                                      values[i][0]);
                }

                if (values[i][1][0] != '-') {
                        test_topic_conf_set(conf, "message.timeout.ms",
                                            values[i][1]);
                }

                rd_kafka_conf_set_default_topic_conf(conf, tconf);

                rk = rd_kafka_new(RD_KAFKA_PRODUCER, conf,
                                  errstr, sizeof(errstr));

                if (values[i][2][0] != '!') {
                        TEST_ASSERT(rk != NULL,
                                    "message timeout linger: %s",
                                    values[i][2]);
                } else {
                        TEST_ASSERT(rk == NULL,
                                    "message timeout linger: %s",
                                    &values[i][2][1]);
                }

                rd_kafka_destroy(rk);
        }
}

[edenhill]

                if (values[i][1][0] != '-') {
                        test_topic_conf_set(conf, "message.timeout.ms",
                                            values[i][1]);
                }

                rd_kafka_conf_set_default_topic_conf(conf, tconf);

It seems like test_topic_conf_set() is called on the conf, not tconf. That should give you a compilation warning and undefined behaviour.

[larry-cdn77]

Indeed, I missed the compile warning. Now with message timeout set in the topic configuration I still get the behaviour, however. The two checks are looking at a topic configuration named linger.ms and linger.ms only exists as a global configuration (specifically here and here).

Once again, I am only curious as to whether the checks could be wrong. I may be misreading the original intent.

[edenhill]

You're absolutely right. I'll change so that .._is_modified() asserts if the property doesn't exist, which will surface this problem in the test suite.

Thank you

[edenhill]

@larry-cdn77 Do you mind if I take your test function?

I also think that this case should actually be a pass, not a fail; it will automatically adjust linger.ms to message.timeout.ms - 0.1:
{"-", "10", "!set M such that L>=M"},

[larry-cdn77]

I would be happy for that test code to be reused

You're right, I had intended to identify case(s) where the auto-adjust comes in and make those an expected pass instead