New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix client IPs being broken on Python 3 #3908

Merged
merged 11 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@hawkowl
Contributor

hawkowl commented Sep 18, 2018

Fixes #3893

hawkowl added some commits Sep 18, 2018

fix

@hawkowl hawkowl requested a review from matrix-org/synapse-core Sep 18, 2018

@erikjohnston

Otherwise looks good

@@ -117,7 +117,7 @@ def _do_execute(self, func, sql, *args):
sql, *args
)
except Exception as e:
logger.debug("[SQL FAIL] {%s} %s", self.name, e)
logger.error("[SQL FAIL] {%s} %s", self.name, e)

This comment has been minimized.

@erikjohnston

erikjohnston Sep 19, 2018

Member

Please don't, as this can get really quite spammy iirc

This comment has been minimized.

@hawkowl

hawkowl Sep 20, 2018

Contributor

From the ticket from @richvdh :

if update_client_ips is failing, it needs to log at ERROR or above.

Do we not want to know when SQL fails?

This comment has been minimized.

@erikjohnston

erikjohnston Sep 20, 2018

Member

Well we raise and handle it later. There are a bunch of failure modes that are non-error case, but instead get retried: https://github.com/matrix-org/synapse/blob/master/synapse/storage/_base.py#L249-L280

I'd really expect the exception to bubble up all the way to the client IP code, so I wonder if we're just swallowing it there?

"Couldn't drop old db: " + str(e), category=UserWarning
)
time.sleep(0.5)

This comment has been minimized.

@erikjohnston

erikjohnston Sep 19, 2018

Member

Can you add quick comment why this is necessary?

)
except Exception as e:
# Failed to upsert, log and continue
logger.error("Failed to insert client IP: %r", entry)

This comment has been minimized.

@richvdh

richvdh Sep 20, 2018

Member

could you log the reason for the failure (ie, e) as well as the fact it's failed?

fix

@hawkowl hawkowl merged commit 1f3f5fc into develop Sep 20, 2018

10 checks passed

ci/circleci: sytestpy2 Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgres Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3 Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgres Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hawkowl hawkowl deleted the hawkowl/py3client-ip branch Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment