Skip to content

Conversation

@xinlian12
Copy link
Member

Second PR for exposingFeedRange in change feed query. In PR #36930, we have exposed feedRange as a str type, but it comes with perf penalty - for each read_feed_ranges response, we would have to always do the base64 encoding, and for each query_items_change_feed using feed_range we have to decode the string.

In this PR, we will expose a FeedRange class type

@xinlian12 xinlian12 requested review from a team and annatisch as code owners September 18, 2024 16:19
@xinlian12
Copy link
Member Author

/azp run python - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

LGTM

container_link: str,
container_rid: str,
change_feed_state_context: Dict[str, Any]):
change_feed_state_context: Dict[str, Any]) -> 'ChangeFeedState':
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it should be a function, not a staticmethod.

}

def _to_feed_range_internal(self) -> 'FeedRangeInternal':
return FeedRangeInternalEpk(self._feed_range)
Copy link
Member

Choose a reason for hiding this comment

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

The method feels a bit redundant - why can't the constructor of FeedRangeInternalEpk just accept an instance of FeedRangeEpk?

@xinlian12 xinlian12 merged this pull request into Azure:users/xinlian/feature/feedRangeAndChangeFeed Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants