Skip to content

refactor: apply some Java best practices #181

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

Merged
merged 10 commits into from
May 3, 2025
Merged

refactor: apply some Java best practices #181

merged 10 commits into from
May 3, 2025

Conversation

KeranYang
Copy link
Member

@KeranYang KeranYang commented Apr 28, 2025

Applying some Java best practices that I learnt from Effective Java. Will selectively apply more as I continue reading.

We no longer need to close FileWriter by ourselves because FileWriter implements java.lang.AutoCloseable. The try-with-resources will automatically close it after finish writing, using the native FileWriter implementation of AutoCloseable.

  • Avoid unnecessary boxing/unboxing for sink response.

In class Response, private final Boolean success; is changed to private final boolean success; such that when we new a Response object with a primitive boolean value, Java doesn't need to perform unnecessary boxing. We don't have a case where success can be null.

* Mark Message getters as protected.

Such that we don't expose them to users. This is technically a backward-incompatible change if an existing user uses getters. Normally, Messages are instantiated right before sending to the next processing unit. I cannot think of a use case for getter. If we don't expect user to use getter, I think we should not expose it.

  • Make DROP_TAGS as a static final.

Such that it's shared by all toDrop calls. We don't need to instantiate a new string array every time user uses toDrop.

  • Use defensive copy when constructing Message objects.

Message is an immutable class. Without defensive copying, user could modify the content of a Message unknowingly by modifying the input reference params.

  • Use private constructor for utility classes.

A utility class should never be instantiated.

  • Mark Message and Response class as final.

We don't expect Message and Response class to be extended.

Copy link

codecov bot commented Apr 28, 2025

Codecov Report

Attention: Patch coverage is 49.37238% with 121 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@1abeb29). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...a/io/numaproj/numaflow/shared/GrpcServerUtils.java 62.50% 7 Missing and 5 partials ⚠️
...numaproj/numaflow/info/ServerInfoAccessorImpl.java 62.96% 7 Missing and 3 partials ⚠️
...main/java/io/numaproj/numaflow/mapper/Message.java 27.27% 5 Missing and 3 partials ⚠️
...java/io/numaproj/numaflow/mapstreamer/Message.java 27.27% 5 Missing and 3 partials ⚠️
...ain/java/io/numaproj/numaflow/sourcer/Message.java 38.46% 4 Missing and 4 partials ⚠️
...o/numaproj/numaflow/sourcetransformer/Message.java 33.33% 5 Missing and 3 partials ⚠️
...java/io/numaproj/numaflow/batchmapper/Message.java 45.45% 3 Missing and 3 partials ⚠️
.../java/io/numaproj/numaflow/mapper/MessageList.java 0.00% 6 Missing ⚠️
...java/io/numaproj/numaflow/reducer/MessageList.java 0.00% 6 Missing ⚠️
...umaproj/numaflow/sessionreducer/model/Message.java 45.45% 3 Missing and 3 partials ⚠️
... and 20 more
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #181   +/-   ##
=======================================
  Coverage        ?   58.76%           
  Complexity      ?      476           
=======================================
  Files           ?      151           
  Lines           ?     3390           
  Branches        ?      231           
=======================================
  Hits            ?     1992           
  Misses          ?     1231           
  Partials        ?      167           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KeranYang KeranYang marked this pull request as ready for review April 28, 2025 14:02
Signed-off-by: Keran Yang <[email protected]>
@KeranYang KeranYang requested a review from RohanAshar April 30, 2025 14:24
@KeranYang KeranYang marked this pull request as draft April 30, 2025 15:53
@KeranYang
Copy link
Member Author

Would like to include this in our next release that work with numaflow 1.5. I am converting this to draft for now. Will merge once I get an approval from @yhl25 .

Copy link
Contributor

@yhl25 yhl25 left a comment

Choose a reason for hiding this comment

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

Please make sure these changes works with both v1.4 and v1.5 version of Numaflow

@KeranYang
Copy link
Member Author

Please make sure these changes works with both v1.4 and v1.5 version of Numaflow

@yhl25 Sure, I will merge this and use numaflow E2E tests to verify that it works with both.

KeranYang added 2 commits May 2, 2025 13:55
@KeranYang KeranYang marked this pull request as ready for review May 3, 2025 15:32
@KeranYang KeranYang merged commit ec9b3bf into main May 3, 2025
5 checks passed
@KeranYang KeranYang deleted the refactor branch May 3, 2025 15:32
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