Skip to content

Conversation

@sky76093016
Copy link
Contributor

@sky76093016 sky76093016 commented Jun 30, 2021

What changes were proposed in this pull request?

Fix negligence issue conditional expressions in line 109.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-5396

How was this patch tested?

No test.

@sky76093016
Copy link
Contributor Author

@jojochuang Please help me review. Thank you.

scmCertStore.getCrls(ImmutableList.of(crlId.get()));

if (crlInfos.isEmpty()) {
if (crlInfos == null || crlInfos.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

When crlInfos can be null?
The value is fetched from. scmCertStore.getCrls(ImmutableList.of(crlId.get())); and this doesn't look like returning a null in any case. It is in general returning an empty list, if it can not find anything?

Secondly, If I understand the code correct, now if crlInfos is null, we will log the line below, so, will this crlInfos.get(0) not throw a NPE since crlInfos is null?

Thirdly, How this null check, preventing an IndexOutOfBoundsException

Copy link
Contributor Author

@sky76093016 sky76093016 Jun 30, 2021

Choose a reason for hiding this comment

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

When crlInfos is empty, crlInfos.get(0) will have something feels unreasonable. After I communicated with @xiaoyuyao , it was because of the original negligence, it should be if (!crlInfos.isEmpty()).

@sky76093016 sky76093016 changed the title HDDS-5396. Fix potential IndexOutOfBoundsException in MockCRLStore.java HDDS-5396. Fix negligence issue conditional expressions in MockCRLStore.java Jun 30, 2021
Copy link
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

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

Looks good now!!!
+1

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

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

I'll merge it

@jojochuang jojochuang merged commit 88383d1 into apache:master Jun 30, 2021
errose28 added a commit to errose28/ozone that referenced this pull request Jul 7, 2021
* master: (28 commits)
  HDDS-5332. Add a new column family and a service provider in Recon DB for Namespace Summaries (apache#2366)
  HDDS-5405. Refactor pom files for HadoopRpc and Grpc/Ratis compilation properties. (apache#2386)
  HDDS-5406. add proto version to all the proto files. (apache#2385)
  HDDS-5398. Avoid object creation in ReplicationManger debug log statements (apache#2379)
  HDDS-5396. Fix negligence issue conditional expressions in MockCRLStore.java (apache#2380)
  HDDS-5395. Avoid unnecessary numKeyOps.incr() call in OMMetrics (apache#2374)
  HDDS-5389. Include ozoneserviceid in fs.defaultFS when configuring o3fs (apache#2370)
  HDDS-5383. Eliminate expensive string creation in debug log messages (apache#2372)
  HDDS-5380. Get more accurate space info for DedicatedDiskSpaceUsage. (apache#2365)
  HDDS-5341. Container report processing is single threaded (apache#2338)
  HDDS-5387. ProfileServlet to move the default output location to an ozone specific directory (apache#2368)
  HDDS-5289. Update container's deleteTransactionId on creation of the transaction in SCM. (apache#2361)
  HDDS-5369. Cleanup unused configuration related to SCM HA (apache#2359)
  HDDS-5381. SCM terminated with exit status 1: null. (apache#2362)
  HDDS-5353. Avoid unnecessary executeBatch call in insertAudits (apache#2342)
  HDDS-5350 :  Add allocate block support in MockOmTransport (apache#2341). Contributed by Uma Maheswara Rao G.
  HDDS-4926. Support start/stop for container balancer via command line (apache#2278)
  HDDS-5269. Datandoe with low ratis log volume space should not be considered for new pipeline allocation. (apache#2344)
  HDDS-5367. Update modification time when updating quota/storageType/versioning (apache#2355)
  HDDS-5352. java.lang.ClassNotFoundException: org/eclipse/jetty/alpn/ALPN (apache#2347)
  ...
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.

3 participants