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

refact: use standard UTF-8 charset & enhance CI configs #2095

Merged
merged 18 commits into from
Mar 7, 2023
Merged

Conversation

imbajin
Copy link
Member

@imbajin imbajin commented Jan 19, 2023

follow the #1632 , and also fix some tiny problem

@zyxxoo could follow this PR to solve the TODOs

@imbajin imbajin added dependencies Incompatible dependencies of package apache labels Jan 19, 2023
@imbajin imbajin added this to the 1.0.0 milestone Jan 19, 2023
@imbajin imbajin requested review from javeme and zyxxoo January 19, 2023 10:13
@imbajin imbajin linked an issue Jan 19, 2023 that may be closed by this pull request
@codecov

This comment was marked as off-topic.

@imbajin
Copy link
Member Author

imbajin commented Feb 1, 2023

TODO: move RAT(check) from build action --> check-license action (fix in #2103)

return buf.bytes();
try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
return buf.bytes();
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems don't need to catch IOException

Copy link
Member Author

Choose a reason for hiding this comment

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

the IOException throws by LZ4Util.compress/decompress & we just auto release the BytesBuffer when it used up, so if we don't catch exception, we need to throws out? (so as the decompress

Copy link
Contributor

Choose a reason for hiding this comment

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

yes just remove the catch, since the LZ4Util.compress already transfer IOException to BackendException

Copy link
Member Author

Choose a reason for hiding this comment

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

yes just remove the catch, since the LZ4Util.compress already transfer IOException to BackendException

�so shall we close the ByteBuffer when it used up? if we remove catch with try(xx), we need throws the exception?

It means we just ignore the ByteBuffer close?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes the BytesBuffer don't need to close, it's just a memory buffer

Copy link
Member Author

@imbajin imbajin Mar 2, 2023

Choose a reason for hiding this comment

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

yes the BytesBuffer don't need to close, it's just a memory buffer

OK, my previous consideration is that if the buffer is large(like compress a big file), we should recycle it manually in time to reduce the pressure of GC & memory usage, but lack enough validation, so back off for now

@imbajin imbajin removed this from the 1.0.0 milestone Feb 10, 2023
return buf.bytes();
try (BytesBuffer buf = LZ4Util.compress(encode(value), BLOCK_SIZE, bufferRatio)) {
return buf.bytes();
} catch (IOException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

yes just remove the catch, since the LZ4Util.compress already transfer IOException to BackendException

@imbajin imbajin changed the title refact: mark TODO in the license problem code & use standard UTF-8 refact: use standard UTF-8 in string code operation Mar 2, 2023
@imbajin imbajin requested a review from z7658329 March 2, 2023 15:57
@imbajin imbajin changed the title refact: use standard UTF-8 in string code operation refact: use standard UTF-8 in string code operation & enhance CI configs Mar 3, 2023
@imbajin imbajin changed the title refact: use standard UTF-8 in string code operation & enhance CI configs refact: use standard UTF-8 charset & enhance CI configs Mar 3, 2023
@imbajin imbajin merged commit 6c0d596 into master Mar 7, 2023
@imbajin imbajin deleted the fix-license branch March 7, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apache dependencies Incompatible dependencies of package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants