Java best practices improvements #486
Conversation
wdroste
commented
Nov 7, 2022
•
wdroste
commented
- Use AirBase for Maven project (reduce common maven patterns)
- Add maven wrapper for easier use by community
- Reduce compile time by removing duplicate compile in test for generated sources
- Use Immutable/fluent pattern for configuration
- Upgrade dependencies
- Migrate from fastjson v1 to v2
- Use spotless to format the source
|
|
|
Uses JDK 11 for AirBase but compiles to JDK 8 target |
| @@ -88,8 +94,7 @@ public static void main(String[] args) { | |||
| NebulaPool pool = new NebulaPool(); | |||
| Session session; | |||
| try { | |||
| NebulaPoolConfig nebulaPoolConfig = new NebulaPoolConfig(); | |||
| nebulaPoolConfig.setMaxConnSize(100); | |||
| NebulaPoolConfig nebulaPoolConfig = NebulaPoolConfig.builder().maxConnSize(100).build(); | |||
There was a problem hiding this comment.
the change for NebulaPoolConfig may cause Incompatible problem for users, may change the usage in the x version.(x.y.z)
There was a problem hiding this comment.
Yes as a consumer of the lib i thought it was fine since its a compile error, easy to fix and provides for immutablility.
There was a problem hiding this comment.
IMO the important parts for this PR is the pom.xml as it upgrades and applys a good standard for the project along w/ spotless which makes the formating easy.
The rest with the configuration is fine either way, but in general it is bad practice to have configuration objects that are mutable. If you can change the configuration while is running that needs to be a feature, but in general I find that it just creates more problems. Its been best practice to make configuration immutable so it can't change in the middle of running it.
There was a problem hiding this comment.
Yeah, the mvnw change is great! And the spotless is convenient for contributors. I'am glad to merge your code with mvnw and spotless.
And for the configuration, I totally agree with the way to build a configuration, I use it in connector too. Since it involves changes on the user side, it needs to be confirmed with pd.
There was a problem hiding this comment.
So could you please split the configuration part to another pr? Then we can merge the mvnw and spotless first.
There was a problem hiding this comment.
Thanks for your pr. But I suggest postponing to the next x version to avoid incompatibility.
