updates to support crate 4.4+ #481

chicco785 · 2021-05-19T06:26:25Z

Proposed changes

Test new version of services: Orion 2.6.1, Crate 4.5.1, Timescale 1.7.5-pg12, Redis 6.2.3.
Updates to code to support changes in Crate 4.4+

Types of changes

What types of changes does your code introduce to the project: Put
an x in the boxes that apply

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing
functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after
creating the PR. If you're unsure about any of them, don't hesitate to
ask. We're here to help! This is simply a reminder of what we are going
to look for before merging your code.

I have read the CONTRIBUTING doc
I have signed the CLA
I have added tests that prove my fix is effective or that my
feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in
downstream modules

Further comments

N/A

github-actions · 2021-05-19T06:26:41Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️

chicco785 · 2021-05-19T07:00:21Z

fix #430 & #306

README.md

run_tests.sh

src/tests/common.py

c0c0n3 · 2021-05-19T12:39:06Z

src/tests/test_integration.py

@@ -46,7 +46,9 @@ def test_integration_basic():
    try:
        entities = load_data()
        assert len(entities) > 1
-        check_data(entities)
+        # it seems data consolidation takes now longer in cratedb
+        time.sleep(20)


sjoe! 20 seconds to actually be able to query data just inserted? Am I missing something here? i.e. is there something else going on under the bonnet besides Crate being slow?

To be honest, I haven't checked if the issue is the notification from orion or the consolidation from crate.
I tend to think the second, because with crate 4.1.x we use shorter time.

oh dear, something we'll need to think about in prod when we upgrade...

Dear Federico and Andrea,

while I am not into the details here, maybe this advise might help you along. Apologies upfront if this does not apply here.

In general you need to refresh a table [1] after inserting some data in order to make data consolidate/synchronize immediately. If that applies to the code here, I am still wondering why it would take 20 seconds, as the default refresh interval is set to 1000 milliseconds.

In any case, we usually invoke REFRESH TABLE foobar; within our integration test scenarios after inserting data into the database [2,3]. Otherwise, the test suite would fail more often than not.

With kind regards,
Andreas.

[1] https://crate.io/docs/crate/reference/en/4.5/general/dql/refresh.html
[2] https://github.com/crate/crate-qa/blob/6630834/tests/client_tests/python/asyncpg/test_asyncpg.py#L38-L42
[3] https://github.com/crate/crate-qa/blob/6630834/tests/client_tests/node-postgres/tests/test_insert_multivalue.js#L15-L19

thx @amotl the test invoked works for both crate and timescaledb, so that's why we don't apply a refresh to crate, probably something clever can be done to force it only for cratedb tests. but i am not sure it is worth. on ci-circle we would just save 20 secs over 8 minutes :)

but even if we don't refresh explicitly just waiting 1 second after the last insert should be enough right?

default refresh interval is set to 1000 milliseconds

or am I misinterpreting the above statement?

src/translators/crate.py

src/translators/sql_translator.py

c0c0n3 · 2021-05-19T12:56:54Z

src/translators/tests/test_crate.py

@@ -25,7 +25,9 @@ def test_geo_point(translator):
    translator.insert([entity])

    # Check location is saved as a geo_point column in crate
-    op = 'select latitude(location), longitude(location) from etroom'
+
+    op = "select latitude(_doc['location']), longitude(_doc['location']) " \


oh dear, what's going on here?! why do we have to pick the location column from _doc now? is it a Crate SQL interface change?

yes :( if you want to have the original precision, and not the approximate one

cool, thanks for the heads up, I've opened an issue about it: #483

Hi again,

I just want to point out #430 (comment) ff. here as a reference why this change happened.

With kind regards,
Andreas.

thx @amotl personally I think the approximate precision will be good enough in most of the cases, so i would not rewrite queries to use the full precision

agreed, I don't think we need sub metre accuracy

src/translators/tests/test_insert_batches.py

c0c0n3 · 2021-05-19T13:05:40Z

@chicco785 excellent job. I've just submitted my review. All the comments there are just questions I asked out of curiosity or to understand what to do about possible issues going forward. No need to actually answer any of them, it's quicker if we talk about that stuff in the next meeting. The only thing I wasn't sure about is the catching of Exception instead of Programming error in the _should_insert_original_entities method. For the rest this PR is good to go in my mind :-)

c0c0n3 · 2021-05-20T15:08:23Z

so it looks like there's some issues w/ our Redis cache, at least w/ Redis 4 which is the one I've been testing the work q with. after turning on the Redis cache (CACHE_QUERIES=True), we started getting alot of Redis errors in the logs like this one

time=2021-05-20 01:11:09.849 | level=WARNING | corr=None | from=172.24.0.1 | srv=None | subserv=None | op=_cache | comp=translators.crate | msg=Caching not available, metadata data may not be consistent: Invalid input of type: 'NoneType'. Convert to a bytes, string, int or float first. | payload=[{'id': 'Room:1', 'type': 'Room', 'temperature': {'value': 23.3, 'type': 'Number'}, 'pressure': {'value': 720, 'type': 'Integer'}, 'time_index': '2021-05-20T13:11:09.838783'}] | thread=140592110414624  | process=10
Traceback (most recent call last):
  File "/src/ngsi-timeseries-api/src/translators/sql_translator.py", line 1585, in _cache
    self.cache.put(tenant_name, key, value, ex)
  File "/src/ngsi-timeseries-api/src/cache/querycache.py", line 26, in put
    self.redis.hset(':' + self.xstr(tenant_name), key, value)
  File "/usr/local/lib/python3.8/site-packages/redis/client.py", line 3050, in hset
    return self.execute_command('HSET', name, *items)
  File "/usr/local/lib/python3.8/site-packages/redis/client.py", line 900, in execute_command
    conn.send_command(*args)
  File "/usr/local/lib/python3.8/site-packages/redis/connection.py", line 725, in send_command
    self.send_packed_command(self.pack_command(*args),
  File "/usr/local/lib/python3.8/site-packages/redis/connection.py", line 775, in pack_command
    for arg in imap(self.encoder.encode, args):
  File "/usr/local/lib/python3.8/site-packages/redis/connection.py", line 119, in encode
    raise DataError("Invalid input of type: '%s'. Convert to a "
redis.exceptions.DataError: Invalid input of type: 'NoneType'. Convert to a bytes, string, int or float first.

it looks like cache entries can't be added up b/c of a type error, but it needs more digging to find out what's actually happening and why...

chicco785 · 2021-05-21T08:29:57Z

so it looks like there's some issues w/ our Redis cache, at least w/ Redis 4 which is the one I've been testing the work q with. after turning on the Redis cache (CACHE_QUERIES=True), we started getting alot of Redis errors in the logs like this one

how this relates with this PR? I think the issue is that service is None

chicco785 · 2021-05-21T08:32:16Z

According to Orion documentation:

The Orion Context Broker implements a simple multitenant/multiservice model based and logical database separation, to ease service/tenant based authorization policies provided by other FIWARE components or third party software, e.g. the ones in the FIWARE security framework (PEP proxy, IDM and Access Control). This functionality is activated when the "-multiservice" command line option is used. When "-multiservice" is used, Orion uses the "Fiware-Service" HTTP header in the request to identify the service/tenant. If the header is not present in the HTTP request, the default service/tenant is used.

so If the header is not present in the HTTP request, the default service/tenant is used. i.e. in the integration there should never be a None tenant

amotl

Thanks a stack for working on this! I also added some comments and hope they are valuable.

c0c0n3 · 2021-05-21T15:56:35Z

@chicco785

how this relates with this PR?

I thought it was a regression issue. In fact, the benchmark tests we've got from back in the day don't use a tenant, but if memory serves, caching was on. I'll have a look next week to see what's the story there.

I think the issue is that service is None

cool thx, I'll check that. But then that means we can't use a cache if QL subscribes for notifications w/ an Orion instance that's not configured for multi tenancy?

c0c0n3

excellent, good to go!

chicco785 requested a review from c0c0n3 May 19, 2021 08:04

c0c0n3 reviewed May 19, 2021

View reviewed changes

c0c0n3 mentioned this pull request May 19, 2021

Possible precision loss w/ Crate 4.4.+ geoqueries #483

Open

chicco785 force-pushed the update-service-version branch from 61a98f9 to 1735976 Compare May 20, 2021 08:37

c0c0n3 mentioned this pull request May 20, 2021

Notification loss under heavy load #416

Closed

Updates to support crate 4.4+

45df4f4

chicco785 force-pushed the update-service-version branch from 1c690ef to 45df4f4 Compare May 21, 2021 08:25

amotl reviewed May 21, 2021

View reviewed changes

c0c0n3 approved these changes May 25, 2021

View reviewed changes

chicco785 merged commit 49d562e into master May 25, 2021

chicco785 deleted the update-service-version branch May 25, 2021 12:41

github-actions bot locked and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updates to support crate 4.4+ #481

updates to support crate 4.4+ #481

chicco785 commented May 19, 2021

github-actions bot commented May 19, 2021 •

edited

Loading

chicco785 commented May 19, 2021 •

edited

Loading

c0c0n3 May 19, 2021

chicco785 May 19, 2021

c0c0n3 May 19, 2021

amotl May 21, 2021

chicco785 May 21, 2021

c0c0n3 May 21, 2021

c0c0n3 May 19, 2021

chicco785 May 19, 2021 •

edited

Loading

c0c0n3 May 19, 2021

amotl May 21, 2021 •

edited

Loading

chicco785 May 21, 2021

c0c0n3 May 21, 2021

c0c0n3 commented May 19, 2021

c0c0n3 commented May 20, 2021

chicco785 commented May 21, 2021

chicco785 commented May 21, 2021 •

edited

Loading

amotl left a comment •

edited

Loading

c0c0n3 commented May 21, 2021

c0c0n3 left a comment

updates to support crate 4.4+ #481

updates to support crate 4.4+ #481

Conversation

chicco785 commented May 19, 2021

Proposed changes

Types of changes

Checklist

Further comments

github-actions bot commented May 19, 2021 • edited Loading

chicco785 commented May 19, 2021 • 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

chicco785 May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c0c0n3 commented May 19, 2021

c0c0n3 commented May 20, 2021

chicco785 commented May 21, 2021

chicco785 commented May 21, 2021 • edited Loading

amotl left a comment • edited Loading

Choose a reason for hiding this comment

c0c0n3 commented May 21, 2021

c0c0n3 left a comment

Choose a reason for hiding this comment

github-actions bot commented May 19, 2021 •

edited

Loading

chicco785 commented May 19, 2021 •

edited

Loading

chicco785 May 19, 2021 •

edited

Loading

amotl May 21, 2021 •

edited

Loading

chicco785 commented May 21, 2021 •

edited

Loading

amotl left a comment •

edited

Loading