Skip to content

Optimize enum class and change some java practice#2435

Merged
qiaojialin merged 2 commits intoapache:masterfrom
yuqi1129:optimize_enum
Jan 22, 2021
Merged

Optimize enum class and change some java practice#2435
qiaojialin merged 2 commits intoapache:masterfrom
yuqi1129:optimize_enum

Conversation

@yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Jan 6, 2021

Description

This PR contains no fix and feature, is about java code optimization and contains the following contents:

  1. Some java practice, for example , remove useless synchronized, remove duplication code and simplify code logic, see below

@HTHou
Copy link
Contributor

HTHou commented Jan 6, 2021

Hi, actually we are working on refactoring the TsFile module and it's almost done. Can you please help us split the modification to TsFile in this PR and submit it to the NewTsFile branch? I think it may save a lot of works... :)
You can check the code here #2184

@jixuan1989
Copy link
Member

jixuan1989 commented Jan 7, 2021

any conflict with newTsfileBranch? @HTHou

@jixuan1989
Copy link
Member

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

code smell!

@HTHou
Copy link
Contributor

HTHou commented Jan 7, 2021

any conflict with newTsfileBranch? @HTHou

Almost the using byte you mentioned.

Copy link
Contributor Author

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

@HTHou @qiaojialin
Thank you for your feedback, i will compare it with #2184 and change accordingly.

@HTHou
Copy link
Contributor

HTHou commented Jan 7, 2021

Hi, I checked the NewTsFile branch carefully. I found the if you change the short to byte in this PR, you may have to modify a lot of code in TsFile module.

There are two ways, one is moving the TsFile part of this PR to another PR commit into NewTsFile. Another is just keeping the short here, once this PR merged into master, I'll resolve the conflicts in NewTsFile.

I think either way is OK. Which one would you like?

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Jan 7, 2021

@HTHou Hi, I think the former is better, it will save us a lot of time and relatively simple.
To make sure, the TsFile part of this PR just means thel enum class changed in this PR ? as other change in this PR is also about in the TsFile project.

@HTHou
Copy link
Contributor

HTHou commented Jan 7, 2021

@HTHou Hi, I think the former is better, it will save us a lot of time and relatively simple.
To make sure, the TsFile part of this PR just means thel enum class changed in this PR ? as other change in this PR is also about in the TsFile project.

What I mean is anything about TsFile module. The change of memtable is another optimization I think :)

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Jan 8, 2021

What I mean is anything about TsFile module. The change of memtable is another optimization I think :)
OK

@yuqi1129
Copy link
Contributor Author

yuqi1129 commented Jan 8, 2021

@HTHou , Changes about TsFile module have been in #2455 ,please take time review it if you are free, thanks

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@qiaojialin qiaojialin merged commit 56b215c into apache:master Jan 22, 2021
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.

4 participants