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

feat(ARCO-291): Ordered callbacks #755

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

boecklim
Copy link
Collaborator

@boecklim boecklim commented Jan 23, 2025

Description of Changes

  • Use ordered send manager to guarantee callbacks are sent in order
  • Refactor callbacker

Testing Procedure

Describe the tests you've added or any testing steps you've taken.

  • I have added new unit tests
  • All tests pass locally
  • I have tested manually in my local environment

Checklist:

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have updated CHANGELOG.md with my changes

@boecklim boecklim self-assigned this Jan 23, 2025
@boecklim boecklim changed the title Feat/ordered callbacks feat: ordered callbacks Jan 23, 2025
@boecklim boecklim force-pushed the feat/ordered-send-manager-batch branch from 7da6924 to 6dad7b5 Compare January 23, 2025 08:36
@boecklim boecklim changed the title feat: ordered callbacks feat(ARCO-291): Ordered callbacks Jan 23, 2025
@boecklim boecklim force-pushed the feat/ordered-callbacks branch 2 times, most recently from 928c7cf to 0c414d7 Compare January 23, 2025 09:26
@boecklim boecklim marked this pull request as ready for review January 23, 2025 09:53
Base automatically changed from feat/ordered-send-manager-batch to main January 28, 2025 13:26
internal/callbacker/processor.go Outdated Show resolved Hide resolved
m.callbackQueue.PushBack(entry)
}

// sortByTimestampAsc sorts the callback queue by timestamp in ascending order
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sorting is working in N^2 time I think. Above 10,000 entries this will be a dead code.

Isn't https://github.com/google/btree an option here? we would simply add items there and not worry about sorting at all. Front would always return smallest (by timestamp).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably this btree package would be faster to sort. But then I need to use another data structure than that linked list which is used here.

There is a maximum of items for the queue so that it cannot have more than 10'000 items:

entriesBufferSize = 10000

I could make sure that this value is not exceeded like this for example

func WithBufferSize(size int) func(*SendManager) {
	return func(m *SendManager) {
		if size >= entriesBufferSize {
			m.bufferSize = entriesBufferSize
			return
		}
		m.bufferSize = size
	}
}

Copy link
Collaborator

@shotasilagadzetaal shotasilagadzetaal Feb 1, 2025

Choose a reason for hiding this comment

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

Why linked list? if you had simply slice and just called sort function every time it would be even faster I think. Anyway, you decide. you can resolve.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added another commit implementing the send manager not using the linked list but a slice

@boecklim boecklim force-pushed the feat/ordered-callbacks branch 2 times, most recently from 1eaf2df to 09bf2cc Compare February 5, 2025 16:09
@boecklim boecklim force-pushed the feat/ordered-callbacks branch from 10cf1ba to 995fcc9 Compare February 6, 2025 09:11
@boecklim boecklim force-pushed the feat/ordered-callbacks branch from 995fcc9 to b3dfacc Compare February 6, 2025 13:41
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