Skip to content

Commit

Permalink
Error rather than warn when an event log query comes in that tries to…
Browse files Browse the repository at this point in the history
… use an int cursor on an event log DB that needs a RunShardedEventsCursor (#7970)

Summary:
Right now, if you pass in an int cursor to a SqliteEventLogStorage query, we warn and then ignore the cursor. The warning incorrectly implies that it will just be slow, but actually the cursor isn't applied at all, which is very unlikely to be what the caller expects. This PR moves that case to an exception instead.

If we're concerned about this being a breaking change we could wait until 0.15.0.
  • Loading branch information
gibsondan committed May 19, 2022
1 parent 2a56fa2 commit e168466
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import sqlite3
import threading
import time
import warnings
from collections import defaultdict
from contextlib import contextmanager
from typing import Iterable, Optional
Expand Down Expand Up @@ -277,15 +276,16 @@ def get_event_records(
else:
asset_details = None

if not event_records_filter or not (
isinstance(event_records_filter.after_cursor, RunShardedEventsCursor)
if (
event_records_filter
and event_records_filter.after_cursor != None
and not isinstance(event_records_filter.after_cursor, RunShardedEventsCursor)
):
warnings.warn(
raise Exception(
"""
Called `get_event_records` on a run-sharded event log storage with a query that
is not run aware (e.g. not using a RunShardedEventsCursor). This likely has poor
performance characteristics. Consider adding a RunShardedEventsCursor to your query
or switching your instance configuration to use a non-run sharded event log storage
Called `get_event_records` on a run-sharded event log storage with a cursor that
is not run-aware. Add a RunShardedEventsCursor to your query filter
or switch your instance configuration to use a non-run-sharded event log storage
(e.g. PostgresEventLogStorage, ConsolidatedSqliteEventLogStorage)
"""
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1339,6 +1339,17 @@ def a_pipe():
]
assert [r.event_log_entry.run_id for r in filtered_records] == ["2", "3"]

# use invalid cursor
with pytest.raises(
Exception, match="Add a RunShardedEventsCursor to your query filter"
):
storage.get_event_records(
EventRecordsFilter(
event_type=DagsterEventType.PIPELINE_SUCCESS,
after_cursor=0,
),
)

def test_watch_exc_recovery(self, storage):
if not self.can_watch():
pytest.skip("storage cannot watch runs")
Expand Down

0 comments on commit e168466

Please sign in to comment.