Skip to content

Add "Buffer" progress bar#685

Merged
KirillOsenkov merged 2 commits intoKirillOsenkov:mainfrom
yuehuang010:main
Jun 2, 2023
Merged

Add "Buffer" progress bar#685
KirillOsenkov merged 2 commits intoKirillOsenkov:mainfrom
yuehuang010:main

Conversation

@yuehuang010
Copy link
Copy Markdown
Contributor

Add a "Buffer" progress bar that will report the BlockingCollection queue length.

image

  • The progress bar title is a placeholder, please help suggest a good title.

Add a "Log Processing Statistics" to report the performance of the each type events.

image

Add a menu option to clear recently opened history.

image

@yuehuang010
Copy link
Copy Markdown
Contributor Author

#684 FYI.

@KirillOsenkov KirillOsenkov merged commit 4f2f8f8 into KirillOsenkov:main Jun 2, 2023
public static Build ReadBuild(string filePath) => ReadBuild(filePath, progress: null);

public static Build ReadBuild(string filePath, Progress progress)
public static Build ReadBuild(string filePath, Progress progress, Progress bufferUsage = null)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Oof, I hadn't realized that this is breaking public API.

Unfortunately I will have to back it out. Sorry about that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No problem. Do revert.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good old overloads are still a thing.

@KirillOsenkov
Copy link
Copy Markdown
Owner

I merged this PR optimistically, but upon reflection I think I'm going to have to revert it.

  1. I'd like to keep the recent items clearing functionality separate from the rest
  2. I'd like to keep the buffer capacity tweaks separate from the rest
  3. we can't change the public API as there are too many tools using this library for reading binlogs and we can't require all of them to update just for a progress bar
  4. our regular users don't need the buffer utilization progress bar nor do they need the Log Processing Statistics
  5. I haven't seen a wall clock time improvement for binlog loading, compared with the previous version. So not sure increasing the buffer is observably helping (at least in the windows case)

@yuehuang010
Copy link
Copy Markdown
Contributor Author

  • I will spin up another PR for the Recent Clear function.
  • I haven't found an improvement with buff capacity either, but that could be a Windows or hardware difference. Is there a way to add a "developer" mode flag?

@KirillOsenkov
Copy link
Copy Markdown
Owner

Don't worry about it, I just pushed the revert:

efb0a57

@KirillOsenkov
Copy link
Copy Markdown
Owner

I would like to avoid regular users paying any perf costs just for developer monitoring tooling.

Your buffer utilization progress bar is good, as well as the perf counters, but stuff like this is good to keep in the branch perhaps. Feel free to make a branch with the revert of efb0a57 and use it for local testing.

Don't think we need this in the main branch.

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.

3 participants