Notifying instead of failing fixes #2111 #2112

juanignaciosl · 2015-02-06T13:14:17Z

@rambo what do you think about this? I think you told me Redis metadata (behind $table_metadata) is no longer used. There're situations like the reported bug when key (table name) is missing and that caused to fail. I think recovering here is legit. I'm not confident about fully removing Redis table metadata usage yet, that's a whole different story now.

What to test: registration was previously done at Redis, and that's been removed. Everything should work fine. With an eye on Rollbar to avoid adding new errors.

Importing.
Manual new table.
Geocoding.
Adding and removing different types of features in a map, and rendering.

Cartofante · 2015-02-06T13:17:52Z

Frontend tests were OK 👍 (details)

…il_because_of_key_on_table_rename

juanignaciosl · 2015-02-09T15:55:54Z

@rambo @Kartones could you please CR this?

Cartofante · 2015-02-09T15:58:04Z

Frontend tests were OK 👍 (details)

Kartones · 2015-02-09T16:15:15Z

+1 and I wonder when we'll fully delete that migrator20.rb and migrator21.rb... probably with 3.0

zenitraM · 2015-02-10T13:58:37Z

The only user of this is/was the tiler, so probably @rochoa can confirm the Redis keys are not used anymore

rochoa · 2015-02-10T14:05:28Z

I'm actually removing old api (/tiles/:table/...), check: CartoDB/Windshaft-cartodb#257

That is using the privacy metadata but since it was already deprecated I don't care too much about those endpoints failing randomly due to that change.

juanignaciosl · 2015-02-10T17:41:28Z

@rochoa : I think we should test this in staging with QA people then so nothing too harmful arises, could you tell me what should we test, related to the tiler? Thanks :-)

rochoa · 2015-02-10T18:30:46Z

If table privacy changes from "public" to "private" it will fail to render the tile either because the privacy is not enough (from the redis metadata) or because it has not enough database privileges.

I will double check the different paths and will write here what to test.

juanignaciosl · 2015-02-10T18:40:54Z

Ok, thank you very much. I'll wait for this to continue. I'm sorry for asking, but could you give an ETA? This Redis error is also raised at common data metadata loading. I was waiting to end this task to enable metadata loading, but if it's taking some days I might patch it instead.

rochoa · 2015-02-10T18:51:07Z

What's that "Redis error is also raised at common data metadata loading" error? I'm not aware of it.

ETA: I will do it tomorrow. 😄

juanignaciosl · 2015-02-10T18:54:58Z

I'm speaking about #2134, I should've linked it before since I spoke about it, sorry 😅
Tomorrow is great, thanks :-)

javisantana · 2015-02-11T07:31:32Z

hey, if the old api fails is not a problem

…il_because_of_key_on_table_rename

Cartofante · 2015-02-11T07:48:23Z

Frontend tests were OK 👍 (details)

juanignaciosl · 2015-02-11T13:51:42Z

@rochoa : @Xatpy found a problem in staging (you can check now at ded01) that I've confirmed in local. This branch makes Varnish invalidation stop working, at least at one case. I've checked the changes and it doesn't seem to be a straightforward problem (I mean, I haven't deleted any call to Varnish invalidation), so you can probably help me or, at least, point to me the place where that cache should be invalidated.

Steps to reproduce:

Open a map.
Create a new polygon with "add new feature" button and "done" for finish.
Expected: polygon is added an appears. What happens: polygon won't appear, but if you zoom, it will. It certainly looks like an invalidation issue (it didn't happen when I was not running Varnish locally).

Thank you in advance :)

PD: I'm trying to reproduce it again in local but I just can't. Nevertheless ded01 won't work.

rochoa · 2015-02-11T14:00:14Z

app/models/table/privacy_manager.rb

      raise 'table privacy cannot be nil' unless privacy
      # TODO: Improve this, hack because tiler checks it
-      $tables_metadata.hset redis_key, 'privacy', privacy_for_redis


BTW I would remove the privacy_for_redis method

zenitraM · 2015-02-11T14:18:48Z

@juanignaciosl let me look at the Varnish issue, it could have been my fault

rochoa · 2015-02-11T14:26:35Z

Things I've been testing:

Preconditions:

Have a table wadus_table with privacy set to private.
You are NOT able to retrieve http://{user}.cartodb.com/tiles/wadus_table/0/0/0.png

Test scenario:

Change privacy in wadus_table from private to public
You CAN retrieve http://{user}.cartodb.com/tiles/wadus_table/0/0/0.png
Change privacy in wadus_table from public back to private
You are NOT able to retrieve http://{user}.cartodb.com/tiles/wadus_table/0/0/0.png

Although we are not hseting the privacy in Redis anymore privacy will continue working because at the end it relies postgres permissions to access the data.

HTH.

juanignaciosl · 2015-02-11T15:20:10Z

@rochoa do you mean that hset of the privacy made it work? :-\

@zenitraM I don't think it's your fault, because rui keeps working.

rochoa · 2015-02-11T15:24:09Z

What I mean is that the hset probably was a 'nice to have' thing at the time but it has no effect because it ends relying on database real permission for the user using the table.

rochoa · 2015-02-11T15:26:15Z

Have a look at this: #1629

juanignaciosl · 2015-02-11T15:31:59Z

It is similar, but it won't appear, no matter how much I wait.

rochoa · 2015-02-11T15:43:37Z

If data changed the {layergroupid}:{last_updated_at} tuple should change so it should not hit anything cached.

Are you using private tables?

juanignaciosl · 2015-02-11T15:57:03Z

I created a new table at both with the defaults for my users, public in staging and private in local, but in local a public one also works. After drawing the polygon I see a difference within two POST requests. This request:
https://juanignaciosl-ded.cartodb-staging.com/api/v1/map?map_key=48d11578f1fce30b3f3a59d131846a99a7f12e37
returns

cdn_url: {http: "api-staging-ded01.cartocdn.com", https: "cartocdn-staging-ded01.global.ssl.fastly.net"}
http: "api-staging-ded01.cartocdn.com"
https: "cartocdn-staging-ded01.global.ssl.fastly.net"
last_updated: "1970-01-01T00:00:00.000Z"
layergroupid: "f4c7eb1388d0fb7cb487c253787af386:0"

but the similar one in local (http://development.localhost.lan/api/v1/map?map_key=cc654eec2c22b45b2a46e4938370530add62a2c5) returns

last_updated: "2015-02-11T15:44:30.368Z"
layergroupid: "79ba7f70276ef1db79c2d9888a26fc1d:1423669470368.13"

In the staging request there's cdn information, and last_updated seems wrong. That request is the last one in staging, in local there're more afterwards that will refresh the content.

juanignaciosl · 2015-02-11T16:47:38Z

For future reference and the rest of you: @rochoa has been helping me with this (thanks!). The problem was ded01 was using a Windshaft version from June. After upgrading it works as expected. Will continue testing then.

Cartofante · 2015-02-11T16:48:20Z

Frontend tests were OK 👍 (details)

rochoa · 2015-02-11T16:49:00Z

Yes, mystery solved! As @juanignaciosl said: Maps API in ded01 machine was running an old version with no x-cache-channel header in layergroups.

Thanks to @zenitraM all staging servers are being deployed, previously only api01-st was being deployed. So I hope no more issues like this inconsistency in staging environment!

juanignaciosl · 2015-02-12T09:44:03Z

Got +1 from QA, I'm merging & deploying.

…n_table_rename Notifying instead of failing fixes #2111

Notifying instead of failing fixes #2111

99edf8d

juanignaciosl added 3 commits February 9, 2015 14:51

Removed Redis table metadata usage fixing #2111

5fd8328

Merge branch 'master' of github.com:CartoDB/cartodb into 2111-Dont_fa…

bd32da7

…il_because_of_key_on_table_rename

Fixed calculated geometry type

3175b8b

rafatower mentioned this pull request Feb 10, 2015

Cache geometry_types in redis #2047 #2165

Merged

Merge branch 'master' of github.com:CartoDB/cartodb into 2111-Dont_fa…

6b50ce4

…il_because_of_key_on_table_rename

rochoa reviewed Feb 11, 2015
View reviewed changes

Removed privacy_for_redis

6ab785f

juanignaciosl added a commit that referenced this pull request Feb 12, 2015

Merge pull request #2112 from CartoDB/2111-Dont_fail_because_of_key_o…

1723204

…n_table_rename Notifying instead of failing fixes #2111

juanignaciosl merged commit 1723204 into master Feb 12, 2015

juanignaciosl mentioned this pull request Feb 12, 2015

No longer use rails:user_id:table_name Redis keys #1884

Closed

juanignaciosl deleted the 2111-Dont_fail_because_of_key_on_table_rename branch August 28, 2015 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifying instead of failing fixes #2111 #2112

Notifying instead of failing fixes #2111 #2112

juanignaciosl commented Feb 6, 2015

Cartofante commented Feb 6, 2015

juanignaciosl commented Feb 9, 2015

Cartofante commented Feb 9, 2015

Kartones commented Feb 9, 2015

zenitraM commented Feb 10, 2015

rochoa commented Feb 10, 2015

juanignaciosl commented Feb 10, 2015

rochoa commented Feb 10, 2015

juanignaciosl commented Feb 10, 2015

rochoa commented Feb 10, 2015

juanignaciosl commented Feb 10, 2015

javisantana commented Feb 11, 2015

Cartofante commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

rochoa Feb 11, 2015

zenitraM commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

rochoa commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

Cartofante commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 12, 2015

Notifying instead of failing fixes #2111 #2112

Notifying instead of failing fixes #2111 #2112

Conversation

juanignaciosl commented Feb 6, 2015

Cartofante commented Feb 6, 2015

juanignaciosl commented Feb 9, 2015

Cartofante commented Feb 9, 2015

Kartones commented Feb 9, 2015

zenitraM commented Feb 10, 2015

rochoa commented Feb 10, 2015

juanignaciosl commented Feb 10, 2015

rochoa commented Feb 10, 2015

juanignaciosl commented Feb 10, 2015

rochoa commented Feb 10, 2015

juanignaciosl commented Feb 10, 2015

javisantana commented Feb 11, 2015

Cartofante commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

rochoa Feb 11, 2015

Choose a reason for hiding this comment

zenitraM commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

rochoa commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

juanignaciosl commented Feb 11, 2015

Cartofante commented Feb 11, 2015

rochoa commented Feb 11, 2015

juanignaciosl commented Feb 12, 2015