deprecate default.topic.configuration #446

rnpridgeon · 2018-09-06T19:42:32Z

No description provided.

edenhill

Great stuff!

some comments.
might want to fix the CI failures in this same PR too

edenhill · 2018-09-06T19:51:07Z

confluent_kafka/src/confluent_kafka.c

@@ -1613,6 +1541,21 @@ rd_kafka_conf_t *common_conf_setup (rd_kafka_type_t ktype,
                PyDict_DelItemString(confdict, "plugin.library.paths");
        }

+        if ((vo = PyDict_GetItemString(confdict, "default.topic.config"))) {
+                PyErr_Warn(PyExc_DeprecationWarning,


Comment out this warning until we hit 1.0

edenhill · 2018-09-06T19:52:14Z

confluent_kafka/src/confluent_kafka.c

+
+                if ((PyDict_Update(confdict, vo) == -1)) {
+                        PyErr_SetString(PyExc_TypeError,
+                                "unable to process default.topic.config");


process is vague, and you shouldn't raise an exception here since PyDict_Update() already did, see the py docs

ack, read the docs(thats how I found PyDict_Update), but failed to consider the fact that it will raise the exception for me

edenhill · 2018-09-06T19:52:24Z

confluent_kafka/src/confluent_kafka.c

+                             "default.topic.config has being deprecated, "
+                             "set default topic configuration values in the global dict");
+
+                if ((PyDict_Update(confdict, vo) == -1)) {


skip outer parens

edenhill · 2018-09-06T19:57:44Z

tests/test_misc.py

+    timeout = time.time() + 5000
+    while not seen_delivery_cb:
+        if time.time() > timeout:
+            if os.environ.get("on_ci") == 'CI':


The environment name is "CI", so:
if "CI" in os.environ:

edenhill · 2018-09-06T19:58:12Z

tests/test_misc.py

+        seen_delivery_cb = True
+        assert err.code() == confluent_kafka.KafkaError._MSG_TIMED_OUT
+
+    p = confluent_kafka.Producer({


Add all three test-cases we discussed:

{"message.timeout.ms":60000, "default.topic.config": {"message.timeout.ms":1000}} -> should take ~1s

{"message.timeout.ms":1000} -> should take ~1s

{"default.topic.config": {"message.timeout.ms":1000}} -> should take ~1s

rnpridgeon · 2018-09-08T01:39:36Z

CI build shows as failing here, but passing in the travis console since the .travis changes have been made. Either way ready for another review @edenhill

edenhill · 2018-09-08T11:05:30Z

tests/test_misc.py

+
+def test_topic_config_update():
+    confs = [{"message.timeout.ms": 600000, "default.topic.config": {"message.timeout.ms": 1000}},
+             {"message.timeout.ms": 1000}, {"default.topic.config": {"message.timeout.ms": 1000}}]


these two should be on separate lines to avoid confusion (I was confused!)

edenhill · 2018-09-08T11:06:10Z

tests/test_misc.py

+        seen_delivery_cb = True
+        assert err.code() == confluent_kafka.KafkaError._MSG_TIMED_OUT
+
+    for conf in confs[:]:


what is the [:] for?

I wanted to be really explicit about the fact its all the configs not just some.

It does the opposite, I'm afraid. Slicing is typically used to get a subset of something.

edenhill · 2018-09-08T11:07:50Z

tests/test_misc.py

+            p.poll(1)
+
+        duration = time.time() - start
+        if 1.02 >= duration <= .98:


this is too tight. Since the "other" timeout is a lot higher (1 minute or 5 minutes), I think any value within 10s is okay here, which also makes it solid on CI.

edenhill

We should change all examples, tests and docs to not use default.topic.config, otherwise cut'n'paste people will keep using it.

edenhill · 2018-09-14T13:11:46Z

confluent_kafka/src/confluent_kafka.c


        /*
         * Default config (overridable by user)
         */

        /* Enable valid offsets in delivery reports */
-        rd_kafka_topic_conf_set(tconf, "produce.offset.report", "true", NULL, 0);
+        rd_kafka_conf_set(conf, "produce.offset.report", "true", NULL, 0);


this is soon being enabled by default (1.0) :)

Want to wait? I can fix the examples in the meantime

No, the default.topic.config fix needs to go in v0.11.6.

edenhill · 2018-09-14T13:15:22Z

..but we can do that for 1.0

edenhill · 2018-09-14T13:15:32Z

LGTM!

edenhill

A few comments.
Also make sure to execute each of the changed files so that they are py-syntactically correct.

edenhill · 2018-09-14T14:14:41Z

docs/index.rst

@@ -110,7 +110,7 @@ https://github.com/edenhill/librdkafka/blob/master/CONFIGURATION.md

 The Python bindings also provide some additional configuration properties:

-* ``default.topic.config``: value is a dict of client topic-level configuration
+* **DEPRECATED** ``default.topic.config``: value is a dict of client topic-level configuration


Needs to mention that topic configuration now goes on the global config dictionary.

edenhill · 2018-09-14T14:15:45Z

examples/integration_test.py

@@ -159,7 +159,7 @@ def verify_producer():
    conf = {'bootstrap.servers': bootstrap_servers,
            'error_cb': error_cb,
            'api.version.request': api_version_request,
-            'default.topic.config': {'produce.offset.report': True}}
+            'produce.offset.report': True}


You can remove this, it is set to true in common_conf_setup()

edenhill · 2018-09-14T14:15:52Z

examples/integration_test.py

@@ -285,7 +285,7 @@ def verify_avro():
    conf = {'bootstrap.servers': bootstrap_servers,
            'error_cb': error_cb,
            'api.version.request': api_version_request,
-            'default.topic.config': {'produce.offset.report': True}}
+            'produce.offset.report': True}


edenhill · 2018-09-14T14:16:14Z

README.md

-    'default.topic.config': {
-        'auto.offset.reset': 'smallest'
-    }
+    'auto.offset.reset': 'smallest'


change smallest -> earliest to be consistent

edenhill · 2018-09-14T14:48:39Z

docs/index.rst

@@ -110,7 +110,8 @@ https://github.com/edenhill/librdkafka/blob/master/CONFIGURATION.md

 The Python bindings also provide some additional configuration properties:

-* **DEPRECATED** ``default.topic.config``: value is a dict of client topic-level configuration
+* **DEPRECATED**: topic configurations should be specified in the global configuration **


the trailing double splats look wrong?

The line now reads as if "topic confiugration (not configruations!) should be specified in the global config .." is DEPRECATED.

I would use something like:
"default.topic.config: value is a dict of client topi-level configuration properties that are applied to all used topics for the instance. DEPRECATED: topic configuration should now be specified in the global top-level configuration

makes sense

edenhill

Needs another two bytes

edenhill · 2018-09-14T19:47:44Z

docs/index.rst

-* **DEPRECATED**: topic configurations should be specified in the global configuration **
-  ``default.topic.config``:value is a dict of client topic-level configuration
-  properties that are applied to all used topics for the instance.
+* ``default.topic.config``:value is a dict of client topic-level configuration


add space after :

edenhill · 2018-09-14T19:48:01Z

docs/index.rst

-  properties that are applied to all used topics for the instance.
+* ``default.topic.config``:value is a dict of client topic-level configuration
+  properties that are applied to all used topics for the instance. **DEPRECATED:**
+  topic configuration should now be specified in the global top-level configuration


end sentence with period.

edenhill

LGTM

as outlined in 34baea6 (confluentinc#446)

deprecate default.topic.configuration

79976c6

rnpridgeon requested a review from edenhill September 6, 2018 19:42

edenhill suggested changes Sep 6, 2018

View reviewed changes

review 1

3aa0b28

edenhill suggested changes Sep 8, 2018

View reviewed changes

rnpridgeon added 2 commits September 9, 2018 19:42

fix tests

b695f51

update travis

9a3ea68

edenhill suggested changes Sep 14, 2018

View reviewed changes

edenhill approved these changes Sep 14, 2018

View reviewed changes

remove default config from examples

98677c6

edenhill suggested changes Sep 14, 2018

View reviewed changes

clean up

506980d

edenhill suggested changes Sep 14, 2018

View reviewed changes

fix doc

e1c46c2

edenhill suggested changes Sep 14, 2018

View reviewed changes

punctuation

0183ac4

edenhill approved these changes Sep 14, 2018

View reviewed changes

rnpridgeon merged commit 34baea6 into master Sep 24, 2018

rnpridgeon deleted the deprecation_topic_conf branch September 24, 2018 19:57

rnpridgeon mentioned this pull request Oct 3, 2018

auto.offset.reset ignored when specified at top level #350

Closed

7 tasks

RasmusWL added a commit to RasmusWL/confluent-kafka-python that referenced this pull request Nov 26, 2018

Remove two default.topic.config uses

447b498

as outlined in 34baea6 (confluentinc#446)

RasmusWL mentioned this pull request Nov 26, 2018

Remove two default.topic.config uses #493

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deprecate default.topic.configuration #446

deprecate default.topic.configuration #446

rnpridgeon commented Sep 6, 2018

edenhill left a comment

edenhill Sep 6, 2018

edenhill Sep 6, 2018

rnpridgeon Sep 6, 2018

edenhill Sep 6, 2018

edenhill Sep 6, 2018

rnpridgeon Sep 6, 2018

edenhill Sep 6, 2018

rnpridgeon commented Sep 8, 2018

edenhill Sep 8, 2018

edenhill Sep 8, 2018

rnpridgeon Sep 8, 2018

edenhill Sep 9, 2018

edenhill Sep 8, 2018

edenhill left a comment

edenhill Sep 14, 2018

rnpridgeon Sep 14, 2018

edenhill Sep 14, 2018

edenhill commented Sep 14, 2018

edenhill commented Sep 14, 2018

edenhill left a comment

edenhill Sep 14, 2018

edenhill Sep 14, 2018

edenhill Sep 14, 2018

edenhill Sep 14, 2018

edenhill Sep 14, 2018

edenhill Sep 14, 2018

rnpridgeon Sep 14, 2018

edenhill left a comment

edenhill Sep 14, 2018

edenhill Sep 14, 2018

edenhill left a comment

deprecate default.topic.configuration #446

deprecate default.topic.configuration #446

Conversation

rnpridgeon commented Sep 6, 2018

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnpridgeon commented Sep 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edenhill commented Sep 14, 2018

edenhill commented Sep 14, 2018

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edenhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edenhill left a comment

Choose a reason for hiding this comment