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

[Improve][Producer] Refactor internalSend() and resouce managment #1113

Closed

Conversation

gunli
Copy link
Contributor

@gunli gunli commented Oct 24, 2023

Fixes #1043

Master Issue:

Motivation

  1. As discussed in [fix] [issue 1051]: Fix inaccurate producer mem limit in chunking and schema #1055, we need to calculate how many pending items and how many memory are required before appending the sendRequest to the dataChan, but currently, we do schema-encoding/compressing in internalSend(), this may lead to inaccurate memory limit cotrolling, and as described in [Improve][Producer]Simplify the MaxPendingMessages controlling #1043, make the code complicated and difficult to maintain, we need to simplify the send logic;
  2. In JAVA client, schema-encoding/compressing are done in application thread, it better to make it work like JAVA client;
  3. As discussed in [fix] [issue 1051]: Fix inaccurate producer mem limit in chunking and schema #1055 and described in [Improve][Producer]Simplify the MaxPendingMessages controlling #1043, resource(memory and semaphore) acquiring/releasing logic are written across the whole file, we need to simplify the resource management logic, encapsulate them into functions, call them when necessary, make it 'Low Coupling, High Cohesion';
  4. As discussed in [Bug][Producer]Inaccurate transaction endSendOrAckOp for chunked message #1060, transaction is not correctly ended for chunked message, it better to encapsulate the transaction ending logic into one func which will be called when sendRequest is done.

Modifications

  1. Move shema encoding from internalSend() to internalSendAsync();
  2. Move compressing from internalSend() to internalSendAsync();
  3. Calculate total chunks before entering the dataChan;
  4. Reserve required semaphore and memory before entering the dataChan;
  5. sendRequest store the semaphore and memory it holds;
  6. Encapsualte relative code blocks into individual funcs to make the skeleton of internalSendAsync() clearer;
  7. Add sendRequest.done() to release the resources it holds;
  8. When a sendRequest is done, call sendRequest.done() ;
  9. In sendRequest.done() run callback, update metrics, end transaction, run interceptors callback...

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as (please describe tests).

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@gunli
Copy link
Contributor Author

gunli commented Oct 24, 2023

@gunli gunli changed the title [PR-1071-3] refactor internalSendAsync [PR-1071-4] refactor internalSendAsync Oct 24, 2023
@gunli gunli marked this pull request as draft October 24, 2023 05:44
@tisonkun tisonkun marked this pull request as ready for review October 24, 2023 10:20
@tisonkun tisonkun mentioned this pull request Oct 24, 2023
1 task
@tisonkun tisonkun changed the title [PR-1071-4] refactor internalSendAsync [Improve][Producer] Refactor internalSend() and resouce managment Oct 24, 2023
@tisonkun tisonkun requested a review from nodece October 24, 2023 10:33
@tisonkun tisonkun marked this pull request as draft October 24, 2023 11:30
@tisonkun tisonkun removed the request for review from nodece October 24, 2023 11:30
@tisonkun
Copy link
Member

Splitted. I'll convey your credit as Co-authored-by.

@tisonkun tisonkun closed this Oct 24, 2023
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.

[Improve][Producer]Simplify the MaxPendingMessages controlling
2 participants