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

Add "Buffer" progress bar #685

Merged
merged 2 commits into from
Jun 2, 2023
Merged

Conversation

yuehuang010
Copy link
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
Contributor Author

#684 FYI.

@KirillOsenkov KirillOsenkov merged commit 4f2f8f8 into KirillOsenkov:main Jun 2, 2023
1 check failed
@@ -27,7 +27,7 @@ public static IEnumerable<Record> ReadRecords(byte[] binlogBytes)

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
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
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
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
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
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
Owner

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

efb0a57

@KirillOsenkov
Copy link
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.

None yet

3 participants