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

Pluginize Persisted Operations #1277

Closed
wants to merge 2 commits into from

Conversation

janeli1
Copy link

@janeli1 janeli1 commented Oct 16, 2024

Motivation and Context

This pull request is to pluginize Persisted Operations.

  1. Expose Persisted Operations interface in pkg directory, this provides flexibility to implement customized logic for FetchPersistedOperations.
  2. Expose ClientInfo interface to give users flexibility to provide customized information. These information can be used as input parameters for Persisted Operations interface or other packages.
  3. Fix the 2nd layer cache issue for Persisted Operations reported here Incorrect ClientInfo Handling When Loading Persisted Operations from Cache #1179. All integration tests changes are due to the changes for this logic.

@StarpTech
Copy link
Contributor

Hi @janeli1,

Thank you for the PR! As a general practice, new features should be discussed in an issue before implementation. This allows us to better understand the reasoning behind them and ensure alignment with the overall direction of the project. For instance, I'm not entirely sure how your proposed additions would be used and what problem they exactly solving.

To help move things forward, could you please separate the fix from the new features and open an issue to discuss them? Apologies for any confusion this process will be made clearer in the future as we plan to improve the PR template.

At the same time, we're currently working on an extensive overhaul of the module system RFC , and I'd appreciate your input on how we might address the points you've included in this PR.

@janeli1
Copy link
Author

janeli1 commented Oct 16, 2024

Sure,

  1. I will create a separate pull request for the 2nd layer of cache and link it under Incorrect ClientInfo Handling When Loading Persisted Operations from Cache #1179
  2. I will open a new issue discussion pluginize Persisted Operations

@janeli1
Copy link
Author

janeli1 commented Oct 16, 2024

I have opened new feature request in #1278 and discussed the reasoning behind this change.

@@ -0,0 +1,65 @@
package clientinfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just make the package client and the interfaces Info and Token?

@@ -664,7 +670,7 @@ type WebSocketConnectionHandler struct {
request *http.Request
conn *wsConnectionWrapper
protocol wsproto.Proto
clientInfo *ClientInfo
detailedClientInfo clientinfo.DetailedClientInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we leave this field name as clientInfo? Then with the interface as client.Info.

@janeli1 janeli1 closed this Oct 17, 2024
@janeli1
Copy link
Author

janeli1 commented Oct 17, 2024

Closed this pull request. As discussed, I will 1) create another pull request for cache logic fix; 2) create a new pull request only contains logic for 1278

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants