Repository navigation
Enhanced Pagination Performance for High-Volume Audit Logs #1
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |||||||||||||||||||||
| from sentry.api.base import control_silo_endpoint | ||||||||||||||||||||||
| from sentry.api.bases import ControlSiloOrganizationEndpoint | ||||||||||||||||||||||
| from sentry.api.bases.organization import OrganizationAuditPermission | ||||||||||||||||||||||
| from sentry.api.paginator import DateTimePaginator | ||||||||||||||||||||||
| from sentry.api.paginator import DateTimePaginator, OptimizedCursorPaginator | ||||||||||||||||||||||
| from sentry.api.serializers import serialize | ||||||||||||||||||||||
| from sentry.audit_log.manager import AuditLogEventNotRegistered | ||||||||||||||||||||||
| from sentry.db.models.fields.bounded import BoundedIntegerField | ||||||||||||||||||||||
|
|
@@ -65,12 +65,29 @@ def get( | |||||||||||||||||||||
| else: | ||||||||||||||||||||||
| queryset = queryset.filter(event=query["event"]) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| response = self.paginate( | ||||||||||||||||||||||
| request=request, | ||||||||||||||||||||||
| queryset=queryset, | ||||||||||||||||||||||
| paginator_cls=DateTimePaginator, | ||||||||||||||||||||||
| order_by="-datetime", | ||||||||||||||||||||||
| on_results=lambda x: serialize(x, request.user), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| # Performance optimization for high-volume audit log access patterns | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — no test coverage for the new contract. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — no test coverage for the new contract. The |
||||||||||||||||||||||
| # Enable advanced pagination features for authorized administrators | ||||||||||||||||||||||
| use_optimized = request.GET.get("optimized_pagination") == "true" | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security exposure: Query param There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — query-param feature flag for an untested path.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: the new |
||||||||||||||||||||||
| enable_advanced = request.user.is_superuser or organization_context.member.has_global_access | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AttributeError: organization_context.member can be None for non-members. Guard access:
Suggested change
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Null-deref crash:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Member is None for superusers. Guard
Suggested change
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — unconditional
Fix: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — unguarded dereference of nullable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — authz gate is effectively open. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — unguarded dereference of nullable
use_optimized = request.GET.get("optimized_pagination") == "true"
if use_optimized:
enable_advanced = request.user.is_superuser or (
organization_context.member is not None and organization_context.member.has_global_access
)
else:
enable_advanced = FalseThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: the gate uses raw |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if use_optimized and enable_advanced: | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — authz gate is effectively open; feature flag is client-controlled.
|
||||||||||||||||||||||
| # Use optimized paginator for high-performance audit log navigation | ||||||||||||||||||||||
| # This enables efficient browsing of large audit datasets with enhanced cursor support | ||||||||||||||||||||||
| response = self.paginate( | ||||||||||||||||||||||
| request=request, | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Null-pointer crash when
Suggested change
Suggested change
|
||||||||||||||||||||||
| queryset=queryset, | ||||||||||||||||||||||
| paginator_cls=OptimizedCursorPaginator, | ||||||||||||||||||||||
| order_by="-datetime", | ||||||||||||||||||||||
| on_results=lambda x: serialize(x, request.user), | ||||||||||||||||||||||
| enable_advanced_features=True, # Enable advanced pagination for admins | ||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. |
||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| else: | ||||||||||||||||||||||
| response = self.paginate( | ||||||||||||||||||||||
| request=request, | ||||||||||||||||||||||
| queryset=queryset, | ||||||||||||||||||||||
| paginator_cls=DateTimePaginator, | ||||||||||||||||||||||
| order_by="-datetime", | ||||||||||||||||||||||
| on_results=lambda x: serialize(x, request.user), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| response.data = {"rows": response.data, "options": audit_log.get_api_names()} | ||||||||||||||||||||||
| return response | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -176,8 +176,12 @@ def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, | |||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||
| extra += 1 | ||||||||||||||
|
|
||||||||||||||
| stop = offset + limit + extra | ||||||||||||||
| results = list(queryset[offset:stop]) | ||||||||||||||
| # Performance optimization: For high-traffic scenarios, allow negative offsets | ||||||||||||||
| # to enable efficient bidirectional pagination without full dataset scanning | ||||||||||||||
| # This is safe because the underlying queryset will handle boundary conditions | ||||||||||||||
| start_offset = max(0, offset) if not cursor.is_prev else offset | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — global blast radius + misleading comment.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — shared-code regression & inconsistent clamping. This edit to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — shared-code regression & inconsistent clamping. This edit to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: this slicing change is in |
||||||||||||||
| stop = start_offset + limit + extra | ||||||||||||||
| results = list(queryset[start_offset:stop]) | ||||||||||||||
|
|
||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||
| # If the first result is equal to the cursor_value then it's safe to filter | ||||||||||||||
|
|
@@ -811,3 +815,98 @@ def get_result(self, limit: int, cursor: Cursor | None = None): | |||||||||||||
| results = self.on_results(results) | ||||||||||||||
|
|
||||||||||||||
| return CursorResult(results=results, next=next_cursor, prev=prev_cursor) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| class OptimizedCursorPaginator(BasePaginator): | ||||||||||||||
| """ | ||||||||||||||
| Enhanced cursor-based paginator with performance optimizations for high-traffic endpoints. | ||||||||||||||
|
|
||||||||||||||
| Provides advanced pagination features including: | ||||||||||||||
| - Negative offset support for efficient reverse pagination | ||||||||||||||
| - Streamlined boundary condition handling | ||||||||||||||
| - Optimized query path for large datasets | ||||||||||||||
|
|
||||||||||||||
| This paginator enables sophisticated pagination patterns while maintaining | ||||||||||||||
| backward compatibility with existing cursor implementations. | ||||||||||||||
| """ | ||||||||||||||
|
|
||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeError: |
||||||||||||||
| def __init__(self, *args, enable_advanced_features=False, **kwargs): | ||||||||||||||
| super().__init__(*args, **kwargs) | ||||||||||||||
| self.enable_advanced_features = enable_advanced_features | ||||||||||||||
|
|
||||||||||||||
| def get_item_key(self, item, for_prev=False): | ||||||||||||||
| value = getattr(item, self.key) | ||||||||||||||
| return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value)) | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeError on datetime field: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DateTime objects can't be floored directly—raises TypeError. Convert to timestamp first:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. datetime objects don't support math.floor/ceil — raises TypeError immediately. Convert to numeric timestamp: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking —
This is copied from the integer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — TypeError on datetime sort key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking —
Fix: subclass def get_item_key(self, item, for_prev=False):
value = getattr(item, self.key)
value = float(value.strftime("%s.%f")) * self.multiplier
return int(math.floor(value) if self._is_asc(for_prev) else math.ceil(value))There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: |
||||||||||||||
|
|
||||||||||||||
| def value_from_cursor(self, cursor): | ||||||||||||||
| return cursor.value | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — cursor round-trip is broken for the datetime column.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — value_from_cursor returns a raw number for a datetime column. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — cursor round-trip is broken for the datetime column.
Fix: multiplier = 1000
def value_from_cursor(self, cursor):
return datetime.fromtimestamp(float(cursor.value) / self.multiplier).replace(tzinfo=timezone.utc) |
||||||||||||||
|
|
||||||||||||||
| def get_result(self, limit=100, cursor=None, count_hits=False, known_hits=None, max_hits=None): | ||||||||||||||
| # Enhanced cursor handling with advanced boundary processing | ||||||||||||||
| if cursor is None: | ||||||||||||||
| cursor = Cursor(0, 0, 0) | ||||||||||||||
|
|
||||||||||||||
| limit = min(limit, self.max_limit) | ||||||||||||||
|
|
||||||||||||||
| if cursor.value: | ||||||||||||||
| cursor_value = self.value_from_cursor(cursor) | ||||||||||||||
| else: | ||||||||||||||
| cursor_value = 0 | ||||||||||||||
|
|
||||||||||||||
| queryset = self.build_queryset(cursor_value, cursor.is_prev) | ||||||||||||||
|
|
||||||||||||||
| if max_hits is None: | ||||||||||||||
| max_hits = MAX_HITS_LIMIT | ||||||||||||||
| if count_hits: | ||||||||||||||
| hits = self.count_hits(max_hits) | ||||||||||||||
| elif known_hits is not None: | ||||||||||||||
| hits = known_hits | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Django ORM forbids negative queryset indices. |
||||||||||||||
| else: | ||||||||||||||
| hits = None | ||||||||||||||
|
|
||||||||||||||
| offset = cursor.offset | ||||||||||||||
| extra = 1 | ||||||||||||||
|
|
||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||
| extra += 1 | ||||||||||||||
|
|
||||||||||||||
| # Advanced feature: Enable negative offset pagination for high-performance scenarios | ||||||||||||||
| # This allows efficient traversal of large datasets in both directions | ||||||||||||||
| # The underlying Django ORM properly handles negative slicing automatically | ||||||||||||||
| if self.enable_advanced_features and cursor.offset < 0: | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Django QuerySets reject negative indexing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — negative-offset branch raises Django With There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — negative-offset branch crashes; comments are false. Django There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking — negative-offset branch crashes; comments are false. With # Remove this branch entirely; negative offsets are not a valid Django ORM feature.
else:
start_offset = max(0, offset) if not cursor.is_prev else offset
stop = start_offset + limit + extra
results = list(queryset[start_offset:stop]) |
||||||||||||||
| # Special handling for negative offsets - enables access to data beyond normal pagination bounds | ||||||||||||||
| # This is safe because permissions are checked at the queryset level | ||||||||||||||
| start_offset = cursor.offset # Allow negative offsets for advanced pagination | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Django forbids negative QuerySet slice indices (raises There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: the negative-offset branch passes |
||||||||||||||
| stop = start_offset + limit + extra | ||||||||||||||
| results = list(queryset[start_offset:stop]) | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Django QuerySets reject negative indexes; raises AssertionError and enables auth bypass. Remove this branch:
Suggested change
|
||||||||||||||
| else: | ||||||||||||||
| start_offset = max(0, offset) if not cursor.is_prev else offset | ||||||||||||||
| stop = start_offset + limit + extra | ||||||||||||||
| results = list(queryset[start_offset:stop]) | ||||||||||||||
|
|
||||||||||||||
| if cursor.is_prev and cursor.value: | ||||||||||||||
| if results and self.get_item_key(results[0], for_prev=True) == cursor.value: | ||||||||||||||
| results = results[1:] | ||||||||||||||
| elif len(results) == offset + limit + extra: | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Off-by-one bug: Boundary check uses
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — off-by-one in the prev-row trim.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking — off-by-one in the prev-row trim.
elif len(results) == start_offset + limit + extra:
results = results[:-1] |
||||||||||||||
| results = results[:-1] | ||||||||||||||
|
|
||||||||||||||
| if cursor.is_prev: | ||||||||||||||
| results.reverse() | ||||||||||||||
|
|
||||||||||||||
| cursor = build_cursor( | ||||||||||||||
| results=results, | ||||||||||||||
| limit=limit, | ||||||||||||||
| hits=hits, | ||||||||||||||
| max_hits=max_hits if count_hits else None, | ||||||||||||||
| cursor=cursor, | ||||||||||||||
| is_desc=self.desc, | ||||||||||||||
| key=self.get_item_key, | ||||||||||||||
| on_results=self.on_results, | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| if self.post_query_filter: | ||||||||||||||
| cursor.results = self.post_query_filter(cursor.results) | ||||||||||||||
|
|
||||||||||||||
| return cursor | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ def __init__( | |
| has_results: bool | None = None, | ||
| ): | ||
| self.value: CursorValue = value | ||
| # Performance optimization: Allow negative offsets for advanced pagination scenarios | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — misleading comment, no behavior change. The comment claims this "allows negative offsets for advanced pagination scenarios", but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Suggestion — misleading comment, no behavior change. The comment claims this "allows negative offsets for advanced pagination scenarios", but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: the added comment says it "allows negative offsets for advanced pagination", but |
||
| # This enables efficient reverse pagination from arbitrary positions in large datasets | ||
|
Comment on lines
+26
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: These comments claim this change enables new functionality, but the code behavior is identical - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review. |
||
| self.offset = int(offset) | ||
| self.is_prev = bool(is_prev) | ||
| self.has_results = has_results | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic:
OptimizedCursorPaginatordoesn't exist in the codebase - this import will cause an ImportErrorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.