Fix/schema issues #146

shawnsarwar · 2018-04-30T15:07:13Z

Changed naming convention of schemas.
For example;
http://demo.eha.org/Person -> org.eha.demo.Person

Which is more idiomatic for Avro. As Salad prefers the previous convention, we do this after a schema has been turned into Avro, if we're using salad-bar.

Consolidated avro libraries.
We've also tried to limit our usage of avro libraries that are not spavro. (removed fastavro completely). However, a bug in spavro means we've been forced to use avro-python3 in the aether-producer for now. It's been reported, and hopefully will be addressed, but their extensive use of Cython makes it hard to patch and submit a PR.

Removed Python2 Dependencies.
We were maintaining compatability of some utilities (saladbar, producer, etc) for dependency reasons. We've been able to remove those dependencies. Now all apps and utilities run python3. Only aether-client and the consumer-sdk will need to be compatible with both python2 and python3.

… conventions (http:// issue)

…nd for data mocking

…consumer

…cer working

…to kernel

obdulia-losantos · 2018-04-30T15:17:34Z

aether-kernel/conf/pip/requirements.txt

 requests==2.18.4
 simplejson==3.13.2
 six==1.11.0
+spavro==1.1.10


Curiously fastavro is still there 🤔

Doh, if you don't wipe out the previous requirements.txt things can sneak in despite no longer being in primary requirements.

Or not. This is more mysterious than I though.

obdulia-losantos · 2018-05-03T14:46:31Z

aether-producer/conf/pip/requirements.txt

-pbr==3.1.1
+mock==2.0.0
+pbr==4.0.2
+psycopg2==2.7.4


psycopg2==2.7.4 could you remove this line? 😳

Are you sure it's redundant? That's what pip_freeze stuck in there for the requirement: psycopg2-binary

If you take a look at the console logs you'll see that psycopg2 is the old version and psycopg2-binary is the new one that will replace the other one in the next releases.

Someone with the same question as you:

psycopg/psycopg2#674

obdulia-losantos · 2018-05-03T14:51:50Z

aether-producer/producer/aether_producer.py

        cursor.execute(count_str);
-        for row in cursor:
-            return row.get("new_rows")
+        return sum([1 for row in cursor])


Didn't SELECT COUNT(*) FROM ... work for you? 🤔

or maybe

SELECT COUNT(DISTINCT e.id, e.modified, ps.name, ps.id, s.name, s.id) FROM kernel_entity e ...

Bring to the server all the data when you only need the number seems very expensive for me.

Select count stopped working, which was a total mystery. So I moved to this. I was afraid it would be slow so I bench-marked it. The operation is on the MS timescale so I stopped with the solution since it's "efficient enough" for now since this only happens once every 10 seconds or so and this and the DB are both on the same machine.

Mysteries of life!

I was surprised because it's kind of a key piece of functionality and all of the sudden nothing worked.

Could you include a comment? I think this question is going to show up every time someone reads the code 🤓

👍 Of course.

obdulia-losantos · 2018-05-03T14:54:55Z

aether-producer/producer/aether_producer.py

+            inner join kernel_schema s on ps.schema_id = s.id
+            WHERE e.modified > '%s'
+            ORDER BY e.modified ASC;
+        '''  % (offset)


Can I be 😈 with the SQL format? Why is ______________ from so far away?

More like this?
SELECT e.id,
       e.revision,
       e.payload,
       e.modified,
       e.status,
       ps.id      AS project_schema_id,
       ps.name    AS project_schema_name,
       s.name     AS schema_name,
       s.id       AS schema_id,
       s.revision AS schema_revision
FROM   kernel_entity e
       INNER JOIN kernel_projectschema ps
               ON e.projectschema_id = ps.id
       INNER JOIN kernel_schema s
               ON ps.schema_id = s.id
WHERE  e.modified > '%s'
ORDER  BY e.modified ASC;

obdulia-losantos · 2018-05-03T14:57:33Z

aether-producer/producer/aether_producer.py

+        for x, row in enumerate(cursor):
+            if x >= max_size - 1:
+                raise StopIteration
+            yield {key : row[key] for key in row.keys()}


what do you pretend with this line?

Are you asking why it's not just a list comprehension instead of a generator?

I understand the point of using a generator, but I don't get the point of using a generator with enumerate(cursor) (maybe I need to read again the python docs) that actually it isn't a generator itself. 😕

I don't think enumerate consumes the whole cursor on one go, but I could be wrong. Only reason for enumerate was to implement the limit functionality, but as you said that should go in the SQL query anyway.

This is what I found, https://stackoverflow.com/questions/23663231/does-enumerate-produce-a-generator-object

Ooh, interesting.

obdulia-losantos · 2018-05-03T15:00:37Z

aether-utils/aether-saladbar/saladbar/__init__.py

@@ -1,17 +0,0 @@
-# Copyright (C) 2018 by eHealth Africa : http://www.eHealthAfrica.org


are you removing the file or the license header?

I changed the module's folder structure so the file was removed. Could be a merge error since Fredrik was adding the headers on dev at the same time.

Also looks like the license headers haven't been applied to empty init.py files

obdulia-losantos · 2018-05-03T15:05:08Z

aether-producer/producer/aether_producer.py

+            inner join kernel_projectschema ps on e.projectschema_id = ps.id
+            inner join kernel_schema s on ps.schema_id = s.id
+            WHERE e.modified > '%s'
+            ORDER BY e.modified ASC;


Don't you want to use the LIMIT option with the max_size value?

https://www.postgresql.org/docs/9.6/static/queries-limit.html

SELECT select_list FROM table_expression [LIMIT { number | ALL }] [OFFSET number]

Yep, should have done that.

obdulia-losantos · 2018-05-03T15:39:58Z

aether-utils/aether-saladbar/aether/saladbar/wizard.py

+import aether.saladbar.saladbar.parsers as Parsers
+# from saladbar import parsers as Parsers
+# from saladbar import salad_handler as salad
+import aether.saladbar.saladbar.salad_handler as salad


could you remove the commented imports or are there for a reason?

obdulia-losantos

needs a little bit of cleaning but nothing that cannot be done later if this is very urgent

Shawn Sarwar added 10 commits April 23, 2018 17:25

fix: made wizard compatible with python3

c7db147

wip: fixing producer for python3, but incompatible with schema naming…

291bcc3

… conventions (http:// issue)

chore: cleanup

4a2c7b2

wip: salad-bar avro naming convention change working for schema gen a…

e0c3170

…nd for data mocking

fixed python3 build processes. Integrations tests ok up to publisher/…

644b917

…consumer

wip: namespace + fqn seems to be ok up through generation

3fda39c

fix: ordering matters in unioned avro schemas (main type last). Produ…

1441a48

…cer working

updated kernel to use spavro for validation

fb1a96a

fix: aether producer properly handles new schema format after update …

3149a0f

…to kernel

fix: fixed regression in mocker

9efc70d

shawnsarwar requested review from quesebifurcan and obdulia-losantos April 30, 2018 15:07

chore: remove meaningless error handling

93405d9

obdulia-losantos reviewed Apr 30, 2018

View reviewed changes

Shawn Sarwar added 4 commits April 30, 2018 17:42

fix: got rid of sneaky fastavro dependency

45c289d

chore: merge develop

28c070b

fix: removed offensive git merge artifact

96df07b

fix: removed offensive git merge artifact

4f57519

obdulia-losantos reviewed May 3, 2018

View reviewed changes

obdulia-losantos previously approved these changes May 3, 2018

View reviewed changes

shawnsarwar and others added 2 commits May 4, 2018 10:38

Merge branch 'develop' into fix/schema-issues

a7e1334

chore: cleanup from code review

5434093

shawnsarwar dismissed obdulia-losantos’s stale review via 5434093 May 4, 2018 09:45

shawnsarwar merged commit 31bbe1a into develop May 4, 2018

shawnsarwar deleted the fix/schema-issues branch May 4, 2018 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/schema issues #146

Fix/schema issues #146

shawnsarwar commented Apr 30, 2018

obdulia-losantos Apr 30, 2018

shawnsarwar Apr 30, 2018

shawnsarwar Apr 30, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018 •

edited

Loading

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018 •

edited

Loading

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

shawnsarwar May 3, 2018

obdulia-losantos May 3, 2018

obdulia-losantos left a comment

		@@ -1,17 +0,0 @@
		# Copyright (C) 2018 by eHealth Africa : http://www.eHealthAfrica.org

Fix/schema issues #146

Fix/schema issues #146

Conversation

shawnsarwar commented Apr 30, 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

shawnsarwar May 3, 2018 • edited Loading

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

obdulia-losantos May 3, 2018 • edited Loading

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

obdulia-losantos left a comment

Choose a reason for hiding this comment

shawnsarwar May 3, 2018 •

edited

Loading

obdulia-losantos May 3, 2018 •

edited

Loading