Skip to content

Conversation

@runzhiwang
Copy link
Contributor

@runzhiwang runzhiwang commented Sep 3, 2020

What changes were proposed in this pull request?

What's the problem ?

image

What's the reason ?

we can not make sure omKeyInfo.getCreationTime() different from omKeyInfo.getModificationTime()

  1. the creationTime was set in OmKeyInfo at TestOMRequestUtils.addKeyToTable -> OmKeyInfo#createOmKeyInfo

  2. the modificationTime was set in KeyArgs at doPreExecute(createAllocateBlockRequest()) -> OMRequest#preExecute.
    And the KeyArgs.modificationTime was set in OmKeyInfo at omAllocateBlockRequest.validateAndUpdateCache -> openKeyInfo.setModificationTime.

  3. Between doPreExecute(createAllocateBlockRequest()) and TestOMRequestUtils.addKeyToTable, there is no time consuming code, so we can not make sure the millisecond time is different.

What is the link to the Apache JIRA

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

How was this patch tested?

no need new test.

@amaliujia
Copy link
Contributor

amaliujia commented Sep 3, 2020

Thanks for the investigation!

I am not sure if this is a good practice: will it help to insert a wait/sleep/a random operation after doPreExecute(createAllocateBlockRequest()); to create the opportunity to differentiate creation time and modification?time

@runzhiwang
Copy link
Contributor Author

@bharatviswa504 Could you help review this patch ? Thank you very much.

Copy link
Contributor

@bharatviswa504 bharatviswa504 left a comment

Choose a reason for hiding this comment

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

Instead of completely removing, can we check greater than or equal to?

@runzhiwang runzhiwang force-pushed the TestOMAllocateBlockRequest branch from 0ab5567 to 6812034 Compare September 3, 2020 06:23
@amaliujia
Copy link
Contributor

In fact, greater than or equal to is a good idea!

@runzhiwang
Copy link
Contributor Author

runzhiwang commented Sep 3, 2020

@bharatviswa504 @amaliujia Thanks for review. I have updated the patch. Besides, I change the order of doPreExecute(createAllocateBlockRequest()) and TestOMRequestUtils.addKeyToTable, to make sure creationTime <= modificationTime, otherwise creationTime >= modificationTime looks weird.

@runzhiwang
Copy link
Contributor Author

@bharatviswa504 CI passed. Could you help merge this patch ? Thank you very much.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @runzhiwang for the fix.

@adoroszlai adoroszlai merged commit c8d5334 into apache:master Sep 3, 2020
@adoroszlai
Copy link
Contributor

Thanks @runzhiwang for fixing this, @amaliujia and @bharatviswa504 for the review.

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