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

Drop backwards-compatibility support for "outlier" #10903

Merged
merged 1 commit into from
Sep 28, 2021
Merged

Drop backwards-compatibility support for "outlier" #10903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/10903.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Drop old functionality which maintained database compatibility with Synapse versions before 1.31.
22 changes: 1 addition & 21 deletions synapse/storage/databases/main/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,13 +1276,6 @@ def _update_outliers_txn(self, txn, events_and_contexts):
logger.exception("")
raise

# update the stored internal_metadata to update the "outlier" flag.
# TODO: This is unused as of Synapse 1.31. Remove it once we are happy
# to drop backwards-compatibility with 1.30.
metadata_json = json_encoder.encode(event.internal_metadata.get_dict())
sql = "UPDATE event_json SET internal_metadata = ? WHERE event_id = ?"
txn.execute(sql, (metadata_json, event.event_id))

# Add an entry to the ex_outlier_stream table to replicate the
# change in outlier status to our workers.
stream_order = event.internal_metadata.stream_ordering
Expand Down Expand Up @@ -1327,19 +1320,6 @@ def event_dict(event):
d.pop("redacted_because", None)
return d

def get_internal_metadata(event):
im = event.internal_metadata.get_dict()

# temporary hack for database compatibility with Synapse 1.30 and earlier:
# store the `outlier` flag inside the internal_metadata json as well as in
# the `events` table, so that if anyone rolls back to an older Synapse,
# things keep working. This can be removed once we are happy to drop support
# for that
if event.internal_metadata.is_outlier():
im["outlier"] = True

return im

self.db_pool.simple_insert_many_txn(
txn,
table="event_json",
Expand All @@ -1348,7 +1328,7 @@ def get_internal_metadata(event):
"event_id": event.event_id,
"room_id": event.room_id,
"internal_metadata": json_encoder.encode(
get_internal_metadata(event)
event.internal_metadata.get_dict()
),
"json": json_encoder.encode(event_dict(event)),
"format_version": event.format_version,
Expand Down
6 changes: 2 additions & 4 deletions synapse/storage/schema/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

# When updating these values, please leave a short summary of the changes below.

SCHEMA_VERSION = 64
SCHEMA_VERSION = 64 # remember to update the list below when updating
"""Represents the expectations made by the codebase about the database schema

This should be incremented whenever the codebase changes its requirements on the
Expand All @@ -35,7 +33,7 @@
"""


SCHEMA_COMPAT_VERSION = 59
SCHEMA_COMPAT_VERSION = 60 # 60: "outlier" not in internal_metadata.
Copy link
Member Author

Choose a reason for hiding this comment

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

strictly speaking, the change to schema version 60 came later than #9411, but near enough.

Copy link
Member

Choose a reason for hiding this comment

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

Should this note be in a comment on this line, or in a Changes in SCHEMA_COMPAT_VERSION = 60: section as above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changes in SCHEMA_COMPAT_VERSION = 60

well, that would be a bad name for the section, since it's not really a change in the "schema compat version" - I'm not even sure that makes any sense. But it's a fair question to ask if it should be in a bit of a longer comment rather than a one-liner here.

What I'm trying to avoid is the situation where somebody bumps the number and forgets to update the comment. I figure if they are on the same line, you have to try extra hard to mess it up. But it does only leave me with about 50 characters, which makes for a rather terse comment.

We could go with a separate comment and also a "# remember to update the comment above" comment, but that feels kinda clunky too.

So, open to suggestions/discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I was mostly just worried about there only being room for a single note here, which would get overwritten in the future when other changes are made.

But are historical changes not a concern here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could match the style of the other comment, but come up with a more accurate heading?

Changes leading to SCHEMA_COMPAT_VERSION = 60:
    - "outlier" is no longer stored in event_json.internal_metadata for backwards compatibility

I do worry that putting a comment on the line has led you to making it a bit terse that I didn't know what you meant until reading the code.

However, wouldn't mind if you thought that we may well not care about the history and the presence of a comment may not particularly matter — git blame can point you to where things are changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

But are historical changes not a concern here?

well no, not really. Once we dropped support for synapses with SCHEMA_VERSION == 60, we'll no longer really care about the reasons we dropped support for synapses with SCHEMA_VERSION == 59. It's the same reason we don't keep a complete history in https://github.com/matrix-org/synapse/blob/develop/synapse/python_dependencies.py: as long as we don't support Twisted 16, who the hell cares why we don't support Twisted 14?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe we don't need a comment here at all. After all, there's always git. I just thought it might be helpful for the curious.

"""Limit on how far the synapse codebase can be rolled back without breaking db compat

This value is stored in the database, and checked on startup. If the value in the
Expand Down