Include C3 monitoring interceptors and update packaging validation #464

rnpridgeon · 2018-10-10T20:07:27Z

No description provided.

edenhill · 2018-10-11T06:33:23Z

confluent_kafka/src/confluent_kafka.c

+ *
+ * @returns 0 on success or -1 on failure (exception raised).
+ */
+static int common_conf_set_special(PyObject *confdict, rd_kafka_conf_t *conf, const char *name, PyObject *vo) {


Try to < 80 columns

use @param vo The property value object

Presumably this is because vo's name and type are fairly ambiguous?

edenhill · 2018-10-11T06:34:20Z

confluent_kafka/src/confluent_kafka.c

+        const char *v;
+        char errstr[256];
+
+        PyObject *vs = NULL, *vs8 = NULL;


all variable definitions in one consecutive block at the head of the function, that is, move this above the empty line.

vs does not need NULL initialization

ack, although is there any harm in initializing it? Here it doesn't matter but in the while loop you will throw uninitialized variable warnings because we jump to inner_error which references them.

Code is documentation. If something is done explicitly the reader thinks there is a reason for it. If there is no reason, we thus shouldn't do it, to avoid confusing the reader.

confluent_kafka/src/confluent_kafka.c

edenhill · 2018-10-11T06:35:40Z

confluent_kafka/src/confluent_kafka.c

+                PyErr_Format(PyExc_TypeError, "expected configuration property %s "
+                             "as type unicode string here", name);
+
+                Py_XDECREF(vs);


this does nothing, vs is always NULL here

edenhill · 2018-10-11T06:36:12Z

confluent_kafka/src/confluent_kafka.c

+                return -1;
+        }
+
+        Py_XDECREF(vs);


Py_DECREF(), we know vs is non-NULL.

Sure, I was thinking consistency might be nice but it unnecessary

Code is also documenting the assumptions of availability, state, etc.
If a variable needs to be checked, then check it.
If we know a variable is always set, don't check it.

edenhill · 2018-10-11T06:39:34Z

confluent_kafka/src/confluent_kafka.c

         */
+         if ((vo = PyDict_GetItemString(confdict, "debug"))) {
+                if (common_conf_set_special(confdict, conf, "debug", vo)) {


no need for braces for single line blocks.

suggest you explicitly check for return value == -1, or change set_special to return 0 on error and 1 on success.

Nope not needed, although arguably it does make it easier to read

edenhill · 2018-10-11T06:40:11Z

confluent_kafka/src/confluent_kafka.c

-        /* Enable valid offsets in delivery reports */
-        rd_kafka_conf_set(conf, "produce.offset.report", "true", NULL, 0);
+        /*
+         * Default config (overridable by user)


This comment doesn't seem to be true anymore.

edenhill · 2018-10-11T06:42:45Z

confluent_kafka/src/confluent_kafka.c

+                /* Resolve plugin paths */
+                PyObject *resolved;
+                resolved = resolve_plugins(vo);
+                if (!resolved) {


remove braces

confluent_kafka/src/confluent_kafka.c

edenhill · 2018-10-11T06:45:33Z

confluent_kafka/src/confluent_kafka.c

+                Py_XDECREF(ks8);
+                Py_XDECREF(ks);
+
+                Py_DECREF(confdict);


goto outer_err here

rnpridgeon · 2018-10-15T13:18:02Z

@edenhill plugged some leaks, fixed some docs. Mind giving it another go

edenhill

tiny miny

edenhill · 2018-10-16T01:59:09Z

confluent_kafka/src/confluent_kafka.c

-
-        /* Enable valid offsets in delivery reports */
-        rd_kafka_conf_set(conf, "produce.offset.report", "true", NULL, 0);
+         if ((vo = PyDict_GetItemString(confdict, "debug")))


micro nit: Personally I would wrap the outer block in { }, or turn it into a single if:
if ((vo = .. ) && !common_set_sp..)) \n goto outer_err

edenhill · 2018-10-16T01:59:52Z

confluent_kafka/src/confluent_kafka.c

                }
                PyDict_DelItemString(confdict, "default.topic.config");
        }

 	/* Convert config dict to config key-value pairs. */
 	while (PyDict_Next(confdict, &pos, &ko, &vo)) {
-		PyObject *ks, *ks8;
+		PyObject *ks = NULL, *ks8 = NULL;


ks does not need initialization

edenhill

There are no tests for the monics using the binary wheels, should be added to test-osx test-manylinux.

edenhill · 2018-10-18T16:30:42Z

.appveyor.yml

@@ -1,6 +1,6 @@
 environment:
  global:
-    LIBRDKAFKA_NUGET_VERSION: 0.11.6-RC3
+    LIBRDKAFKA_NUGET_VERSION: 0.11.6-RC4


Suggested change

LIBRDKAFKA_NUGET_VERSION: 0.11.6-RC4

LIBRDKAFKA_NUGET_VERSION: 0.11.6-RC5

edenhill · 2018-10-18T16:31:57Z

.travis.yml

@@ -1,32 +1,32 @@
 matrix:
 include:
-  # Source package verification with Python 2.7 and librdkafka v0.11.6-RC3
+  # Source package verification with Python 2.7 and librdkafka v0.11.6-RC4


RC4 -> RC5 across the board

edenhill · 2018-10-18T16:32:53Z

.travis.yml

- - if [[ -z $CIBW_BEFORE_BUILD ]]; then py.test -v --timeout 20 --ignore=tmp-build --import-mode append ; fi
- - if [[ -n $TRAVIS_TAG && -n $CIBW_BEFORE_BUILD ]]; then cibuildwheel --output-dir wheelhouse ; ls -la wheelhouse/ ; fi
+ # Make plugins available for tests
+ - ldd staging/libs/* || true ; otool -L staging/libs/* || true


Suggested change

- ldd staging/libs/* || true ; otool -L staging/libs/* || true

- ldd staging/libs/* || otool -L staging/libs/* || true

edenhill · 2018-10-18T16:37:38Z

tests/test_misc.py

+            f = os.path.join(path, "monitoring-interceptor" + ext)
+            if os.path.exists(f):
+                return False
+            print('Not found: {}'.format(f))


Suggested change

print('Not found: {}'.format(f))

edenhill · 2018-10-18T16:38:42Z

tools/RELEASE.md


-    $ git commit -m "librdkafka version bump to v0.11.6" .travis.yml .appveyor.yml
+ * `tools/install-interceptors.sh`


Suggested change

* `tools/install-interceptors.sh`

* `tools/install-interceptors.sh` - edit and change version

Just above this we state the following:

"Change to the latest version of the confluent-librdkafka-plugins in:"

To be clear this in addition to this statement not a replacement correct?

I just wanted to clarify that the file needs editing rather than being run, since referencing a shell script in backticks usually suggests the latter.

sure works for me just wanted to make sure

edenhill · 2018-10-18T18:53:41Z

tools/test-osx.sh

@@ -34,15 +34,15 @@ pip install confluent_kafka --no-cache-dir --no-index -f $WHEELHOUSE
 # https://github.com/pypa/pip/issues/5247
 pip install pytest pytest-timeout --ignore-installed six

-# Verify that OpenSSL and zlib are properly linked


I tthink we should do cd .. here prior to all the remaining steps

RasmusWL · 2019-01-22T13:57:12Z

I cannot stress how happy I am about this. I know it is an awful practise to make +1 comments on issues, but I really want to show my appreciation to you guys @rnpridgeon and @edenhill 💖 🎉

You have officially made my day! 🥇

rnpridgeon requested a review from edenhill October 10, 2018 20:07

edenhill changed the title ~~Update packaging validation~~ Include C3 monitoring interceptors and update packaging validation Oct 11, 2018

edenhill suggested changes Oct 11, 2018

View reviewed changes

edenhill suggested changes Oct 16, 2018

View reviewed changes

edenhill suggested changes Oct 18, 2018

View reviewed changes

edenhill approved these changes Oct 18, 2018

View reviewed changes

edenhill suggested changes Oct 18, 2018

View reviewed changes

edenhill force-pushed the pkg-monic branch 2 times, most recently from 2442325 to 0b2cce3 Compare October 19, 2018 00:27

edenhill and others added 3 commits October 19, 2018 11:02

Add confluent monitoring-interceptors to wheels

aa1958f

produce.offset.report=true is no longer needed

b8ced77

Intercept and set debug context before interceptor

729e0de

edenhill force-pushed the pkg-monic branch from b7136d3 to 729e0de Compare October 19, 2018 18:02

edenhill merged commit b4a4d9a into master Oct 19, 2018

edenhill deleted the pkg-monic branch October 19, 2018 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include C3 monitoring interceptors and update packaging validation #464

Include C3 monitoring interceptors and update packaging validation #464

rnpridgeon commented Oct 10, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

rnpridgeon Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

rnpridgeon Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

rnpridgeon Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

rnpridgeon Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

edenhill Oct 11, 2018

rnpridgeon Oct 11, 2018

rnpridgeon commented Oct 15, 2018

edenhill left a comment

edenhill Oct 16, 2018

edenhill Oct 16, 2018

edenhill left a comment

edenhill Oct 18, 2018

edenhill Oct 18, 2018

edenhill Oct 18, 2018

edenhill Oct 18, 2018

edenhill Oct 18, 2018

rnpridgeon Oct 18, 2018

edenhill Oct 18, 2018

rnpridgeon Oct 18, 2018

edenhill Oct 18, 2018

RasmusWL commented Jan 22, 2019

	LIBRDKAFKA_NUGET_VERSION: 0.11.6-RC4
	LIBRDKAFKA_NUGET_VERSION: 0.11.6-RC5

	- ldd staging/libs/* \|\| true ; otool -L staging/libs/* \|\| true
	- ldd staging/libs/* \|\| otool -L staging/libs/* \|\| true


		$ git commit -m "librdkafka version bump to v0.11.6" .travis.yml .appveyor.yml
		* `tools/install-interceptors.sh`

	* `tools/install-interceptors.sh`
	* `tools/install-interceptors.sh` - edit and change version

Include C3 monitoring interceptors and update packaging validation #464

Include C3 monitoring interceptors and update packaging validation #464

Conversation

rnpridgeon commented Oct 10, 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

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

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 Oct 15, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL commented Jan 22, 2019