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

Remove JDK 1.8 specific functions from tikv-client #2761

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frew
Copy link

@frew frew commented Aug 25, 2023

What problem does this PR solve?

Removes JDK 1.8 specific code.

What is changed and how it works?

Mostly the change doesn't affect functionality - we pull in javax.annotation-api from Maven which should work for both JDK 1.8 and later versions and remove unused code in MemoryUtil.

The one functionality change is removing MemoryUtil.free calls from TiBlockColumnVector and AutoGrowByteBuffer. This will defer buffer removal from the close() call to the finalize() call on the relevant object, but there doesn't appear to be a clean way independent of JDK versions to do this. I've tested some with Spark and it doesn't appear to have a major impact on memory usage.

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    Most changes are verified by compilation passing. Memory changes have been verified by manually running some Spark jobs.

Code changes

  • Has exported function/method change
    Some MemoryUtil methods have been removed, but that's intended to be internal.

Side effects

  • Possible performance regression

Possible that memory usage will change due to free() call removal.

Related changes

None of the above

@ti-chi-bot
Copy link

ti-chi-bot bot commented Aug 25, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign humengyu2012 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sre-bot
Copy link
Contributor

sre-bot commented Aug 25, 2023

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot
Copy link

ti-chi-bot bot commented Aug 25, 2023

Welcome @frew! It looks like this is your first PR to pingcap/tispark 🎉

@ti-chi-bot ti-chi-bot bot added the size/M label Aug 25, 2023
@xuanyu66
Copy link
Collaborator

Currently, we don't have a plan to support JDK(>1.8), it's a huge work.

@frew
Copy link
Author

frew commented Aug 30, 2023

@xuanyu66 Apologies if I'm missing something, but it seems like this PR makes it work? I've at least compiled TiSpark on JDK 11 with this PR and used it to export >10 TB of data successfully. Am I missing a use case that requires the unsafe free code?

@xuanyu66
Copy link
Collaborator

@frew Thanks for contributing. We will keep an eye on this and find time to do a thorough test

@shiyuhang0
Copy link
Member

/run-all-tests

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.

4 participants