Skip to content

Conversation

@smitajoshi12
Copy link
Contributor

@smitajoshi12 smitajoshi12 commented Aug 6, 2024

What changes were proposed in this pull request?

Optinal Chaining for Undefined Checks

Please describe your PR in detail:

Currently we are using explicit undefined checks for statements.
[Sample|https://github.com/apache/ozone/blob/dcfa3b42ba830781de589d13b30162a426ad3a3d/hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/views/missingContainers/missingContainers.tsx#L401]:
{code:javascript}
Missing${(missingDataSource && missingDataSource.length > 0) ? (${missingDataSource.length})` : ''}
{code}
This causes lots of unnecessary code to be written as guard clauses.
JavaScript offers optional chaining feature which would allow us to skip such checks and make our code more readable.
Optional chaining will short-circuit the check and return undefined if at any step the accessed property is undefined so we do not need to handle so many checks.

The above sample code can be written as:
{code:javascript}
Missing${(missingDataSource?.length > 0) ? (${missingDataSource.length})` : ''}
{code}
where if missingDataSource is undefined, then the expression evaluates to:
{code:javascript}
(undefined > 0)
{code}
which will return false and hence achieve the same result in a more compact syntax

What is the link to the Apache JIRA

(https://issues.apache.org/jira/browse/HDDS-11246)

How was this patch tested?

Manually

…ed check for Objects in Container and Pipeline Module
@smitajoshi12 smitajoshi12 changed the title HDDS-11246. [Recon] Use optional chaining instead of explicit undefin… [Recon] Use optional chaining instead of explicit undefined check for Objects in Container and Pipeline Module Aug 6, 2024
Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

Hi @smitajoshi12, thanks for taking this effort to improve the code quality.
I had a few minor reviews. Please take a look

…ed check for Objects in Container and Pipeline Module
@devabhishekpal
Copy link
Contributor

Thanks for the changes @smitajoshi12, just another minor change.
Can we use 0 instead of null string?
As in the current change it will show as Missing Containers () in case the data is undefined
Instead it would be better to show it as Missing Containers (0)

Example:
Missing (${missingDataSource?.length ?? 0})

…ed check for Objects in Container and Pipeline Module Review Changes
@smitajoshi12
Copy link
Contributor Author

Thanks for the changes @smitajoshi12, just another minor change. Can we use 0 instead of null string? As in the current change it will show as Missing Containers () in case the data is undefined Instead it would be better to show it as Missing Containers (0)

Example: Missing (${missingDataSource?.length ?? 0})

@devabhishekpal
Done changes in latest commit. 639e4b1

Copy link
Contributor

@devabhishekpal devabhishekpal left a comment

Choose a reason for hiding this comment

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

Thanks @smitajoshi12
These changes look good to me, +1

@devabhishekpal
Copy link
Contributor

Hi @smitajoshi12 , can you please change the heading?

HDDS-11246. Use optional chaining for object undefined checks in Pipelines

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch @smitajoshi12
LGTM +1

@ArafatKhan2198
Copy link
Contributor

@smitajoshi12 please check your fork and resolve the failures in the your CI

@smitajoshi12
Copy link
Contributor Author

smitajoshi12 commented Aug 13, 2024

@smitajoshi12 please check your fork and resolve the failures in the your CI

@ArafatKhan2198
After Merge With Master CI Build looks good. You can merge it.

@ArafatKhan2198 ArafatKhan2198 changed the title [Recon] Use optional chaining instead of explicit undefined check for Objects in Container and Pipeline Module HDDS-11246. [Recon] Use optional chaining instead of explicit undefined check for Objects in Container and Pipeline Module. Aug 14, 2024
@ArafatKhan2198
Copy link
Contributor

Thanks for the improvement @smitajoshi12
Thanks for the review @devabhishekpal

@ArafatKhan2198 ArafatKhan2198 merged commit bb80a14 into apache:master Aug 19, 2024
errose28 added a commit to errose28/ozone that referenced this pull request Aug 21, 2024
* master: (50 commits)
  HDDS-11331. Fix Datanode unable to report for a long time (apache#7090)
  HDDS-11346. FS CLI gives incorrect recursive volume deletion prompt (apache#7102)
  HDDS-11349. Add NullPointer handling when volume/bucket tables are not initialized (apache#7103)
  HDDS-11209. Avoid insufficient EC pipelines in the container pipeline cache (apache#6974)
  HDDS-11284. refactor quota repair non-blocking while upgrade (apache#7035)
  HDDS-9790. Add tests for Overview page (apache#6983)
  HDDS-10904. [hsync] Enable PutBlock piggybacking and incremental chunk list by default (apache#7074)
  HDDS-11322. [hsync] Block ECKeyOutputStream from calling hsync and hflush (apache#7098)
  HDDS-11325. Intermittent failure in TestBlockOutputStreamWithFailures#testContainerClose (apache#7099)
  HDDS-11340. Avoid extra PubBlock call when a full block is closed (apache#7094)
  HDDS-11155. Improve Volumes page UI (apache#7048)
  HDDS-11324. Negative value preOpLatencyMs in DN audit log (apache#7093)
  HDDS-11246. [Recon] Use optional chaining instead of explicit undefined check for Objects in Container and Pipeline Module. (apache#7037)
  HDDS-11323. Mark TestLeaseRecovery as flaky
  HDDS-11338. Bump zstd-jni to 1.5.6-4 (apache#7085)
  HDDS-11337. Bump Spring Framework to 5.3.39 (apache#7084)
  HDDS-11327. [hsync] Revert config default ozone.fs.hsync.enabled to false (apache#7079)
  HDDS-11325. Mark testWriteMoreThanMaxFlushSize as flaky
  HDDS-11336. Bump slf4j to 2.0.16 (apache#7086)
  HDDS-11335. Bump exec-maven-plugin to 3.4.1 (apache#7087)
  ...

Conflicts:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
errose28 added a commit to errose28/ozone that referenced this pull request Aug 21, 2024
* master: (50 commits)
  HDDS-11331. Fix Datanode unable to report for a long time (apache#7090)
  HDDS-11346. FS CLI gives incorrect recursive volume deletion prompt (apache#7102)
  HDDS-11349. Add NullPointer handling when volume/bucket tables are not initialized (apache#7103)
  HDDS-11209. Avoid insufficient EC pipelines in the container pipeline cache (apache#6974)
  HDDS-11284. refactor quota repair non-blocking while upgrade (apache#7035)
  HDDS-9790. Add tests for Overview page (apache#6983)
  HDDS-10904. [hsync] Enable PutBlock piggybacking and incremental chunk list by default (apache#7074)
  HDDS-11322. [hsync] Block ECKeyOutputStream from calling hsync and hflush (apache#7098)
  HDDS-11325. Intermittent failure in TestBlockOutputStreamWithFailures#testContainerClose (apache#7099)
  HDDS-11340. Avoid extra PubBlock call when a full block is closed (apache#7094)
  HDDS-11155. Improve Volumes page UI (apache#7048)
  HDDS-11324. Negative value preOpLatencyMs in DN audit log (apache#7093)
  HDDS-11246. [Recon] Use optional chaining instead of explicit undefined check for Objects in Container and Pipeline Module. (apache#7037)
  HDDS-11323. Mark TestLeaseRecovery as flaky
  HDDS-11338. Bump zstd-jni to 1.5.6-4 (apache#7085)
  HDDS-11337. Bump Spring Framework to 5.3.39 (apache#7084)
  HDDS-11327. [hsync] Revert config default ozone.fs.hsync.enabled to false (apache#7079)
  HDDS-11325. Mark testWriteMoreThanMaxFlushSize as flaky
  HDDS-11336. Bump slf4j to 2.0.16 (apache#7086)
  HDDS-11335. Bump exec-maven-plugin to 3.4.1 (apache#7087)
  ...

Conflicts:
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants