Skip to content
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

Specify readerID on loading history tasks #3994

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

yycptt
Copy link
Member

@yycptt yycptt commented Feb 28, 2023

What changed?

  • Specify readerID on loading history tasks

Why?

How did you test it?

  • Unit test

Potential risks

  • May cause additional loading when readerID changes on a iterator as buffered tasks inside the paging iterator will be discarded.

Is hotfix candidate?
no

@yycptt yycptt requested a review from a team as a code owner February 28, 2023 00:06
Comment on lines +36 to +37
HasNext(readerID int32) bool
Next(readerID int32) (tasks.Task, error)
Copy link
Member Author

Choose a reason for hiding this comment

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

I want iterators to be able to freely move between slices & queue readers, instead of bind to a specific slice or reader. Existing implementation is also based on that idea.

So readerID here can be regarded as a context for loading operations. The iterator implementation knows that if readerID changes, a new underlying pagingIterator is needed for talking to persistence.

@yycptt yycptt requested review from yiminc, yux0 and pdoerner February 28, 2023 00:12
@yycptt yycptt force-pushed the get-tasks-reader-id branch from 184a5cb to 723ce47 Compare March 1, 2023 22:53
@yycptt yycptt changed the base branch from queue-reader-persistence-api to master March 2, 2023 19:49
@yycptt yycptt force-pushed the get-tasks-reader-id branch from 723ce47 to 3149281 Compare March 2, 2023 19:51
@@ -257,6 +258,7 @@ func (r *dlqHandlerImpl) readMessagesWithAckLevel(
GetHistoryTasksRequest: persistence.GetHistoryTasksRequest{
ShardID: r.shard.GetShardID(),
TaskCategory: tasks.CategoryReplication,
ReaderID: common.DefaultQueueReaderID,
Copy link
Member Author

Choose a reason for hiding this comment

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

This field probably doesn't make sense for replication DLQ.

I may later refactor the code so that GetReplicationDLQTaskRequest no longer embeds GetHistoryTasksRequest. May also do it for those dlq requests, haven't decided yet. We can sync later.
Not a problem right now cause persistence impl won't look at this field.

cc @yux0

@yycptt yycptt merged commit 701b775 into temporalio:master Mar 2, 2023
@yycptt yycptt deleted the get-tasks-reader-id branch March 2, 2023 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants