Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Don't add rejections to the state_group, persist all rejections #948

Merged
merged 5 commits into from
Jul 26, 2016

Conversation

NegativeMjark
Copy link
Contributor

No description provided.

@erikjohnston
Copy link
Member

erikjohnston commented Jul 25, 2016

LGTM

Err, i take that back

@richvdh
Copy link
Member

richvdh commented Jul 26, 2016

I can't speak to the correctness of the changes themselves, but in my experience, when stuff starts getting complicated, lots of comments explaining what is going on can really help. I'm surprised to see quite a lot of code here, with little attention to comments explaining why things are doing what they are doing.

@@ -442,6 +433,9 @@ def _persist_events_txn(self, txn, events_and_contexts, backfilled):
# Handle the case of the list including the same event multiple
# times. The tricky thing here is when they differ by whether
# they are an outlier.
if context.rejected:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code appears to have landed between a comment and the code it refers to, thus making the comment confusing. Because this method wasn't confusing enough before, I suppose.

@erikjohnston
Copy link
Member

I guess the main changes here are: don't add rejected events to event forward extremities table (and don#t and them to the push actions).

@erikjohnston
Copy link
Member

Although I'm slightly concerned that we have stopped storing various bits of information about the event, and how that affects other storage functions. I'd prefer if, for now, the changes were as minimal as possible so that we can get the fix out and have refactoring (and commenting) as a separate PR.

@NegativeMjark NegativeMjark merged commit 9c4cf83 into develop Jul 26, 2016
@richvdh richvdh deleted the markjh/auth_fixes branch December 1, 2016 14:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants