Skip to content

Conversation

@martijnimhoff
Copy link
Contributor

The incremental assign contained a memory leak. This happened because when unassigning, the wrong new pointers are destroyed (m_partitions).

Instead the partitions argument should be destroyed, because this is only used to find the right toppars in m_partitions. And the toppars in m_partitions which are unassigned also should be destroyed.

@martijnimhoff martijnimhoff requested a review from a team as a code owner April 11, 2024 16:33
@milindl milindl self-requested a review April 12, 2024 05:46
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just a bunch of minor changes, looks good otherwise.

@martijnimhoff martijnimhoff requested a review from milindl April 16, 2024 14:37
Copy link
Contributor

@milindl milindl left a comment

Choose a reason for hiding this comment

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

Approving and running CI

@milindl
Copy link
Contributor

milindl commented Apr 17, 2024

/sem-approve

@martijnimhoff
Copy link
Contributor Author

I didn't push properly, so the comments weren't added, but that is done now!

@milindl
Copy link
Contributor

milindl commented Apr 18, 2024

@martijnimhoff it looks good to me. As this is our first external PR with code changes, there's a bunch of minor things I'm doing to get the CI to run on this once, and there should be a message from our CLA bot in a while on this PR for you. Thanks!

@milindl
Copy link
Contributor

milindl commented Apr 18, 2024

Here's the link to the CLA that should be signed: https://cla-assistant.io/confluentinc/confluent-kafka-javascript?pullRequest=35

@cla-assistant
Copy link

cla-assistant bot commented Apr 18, 2024

CLA assistant check
All committers have signed the CLA.

@martijnimhoff
Copy link
Contributor Author

I've signed it. Should be fine now?

@milindl
Copy link
Contributor

milindl commented Apr 20, 2024

/sem-approve

@milindl
Copy link
Contributor

milindl commented Apr 22, 2024

Closing and reopening PR to try and trigger semaphore CI checks to run.

@milindl milindl closed this Apr 22, 2024
@milindl milindl reopened this Apr 22, 2024
@milindl
Copy link
Contributor

milindl commented Apr 22, 2024

/sem-approve

@milindl milindl merged commit 6118b66 into confluentinc:dev_early_access_development_branch Apr 22, 2024
@milindl
Copy link
Contributor

milindl commented Apr 22, 2024

Thanks @martijnimhoff for the fix!

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