Skip to content
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.

Sign up for GitHub

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

Jump to bottom

HDDS-5315. Add basic unit tests for skipping tokens #2329

Closed
wants to merge 1 commit into from
Closed

HDDS-5315. Add basic unit tests for skipping tokens #2329

wants to merge 1 commit into from

Conversation

kerneltime
Copy link
Contributor

What changes were proposed in this pull request?

Adding a basic unit test to validate tokens are skipped when persisting.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tested

@kerneltime kerneltime changed the title Hdds 5315 Hdds 5315 Add basic unit tests for skipping tokens Jun 11, 2021
@kerneltime kerneltime force-pushed the HDDS-5315 branch 4 times, most recently from b213953 to da21951 Compare June 11, 2021 19:47
@kerneltime kerneltime mentioned this pull request Jun 11, 2021
Merged
1 task
@kerneltime kerneltime force-pushed the HDDS-5315 branch 4 times, most recently from dc266ad to 08a6ba6 Compare June 11, 2021 20:03
@adoroszlai
Copy link
Contributor

@kerneltime

  • You can find checkstyle violations in the output of the failing check.
  • Instead of force pushing please prefer incremental commits to fix issues after you have opened the PR. These make CI history more easily accessible.

@kerneltime kerneltime force-pushed the HDDS-5315 branch 2 times, most recently from 361e8dd to ddd781d Compare June 11, 2021 20:59
@kerneltime
Copy link
Contributor Author

@kerneltime

  • You can find checkstyle violations in the output of the failing check.
  • Instead of force pushing please prefer incremental commits to fix issues after you have opened the PR. These make CI history more easily accessible.

I incorporated the check style scripts that are part of the source code and the checks pass locally as well as here.
Not sure I follow which part of the CI history would you like to refer to.

@cxorm
Copy link
Member

cxorm commented Jun 12, 2021 •
edited
Loading

Hi @kerneltime, thank you for the work.

Could you please file a new jira to address this unit test ?
I think it make sense to file a new jira if you don't mind.

@adoroszlai
Copy link
Contributor

Not sure I follow which part of the CI history would you like to refer to.

GitHub runs CI checks for the latest commit on the branch when pushed. If you address issues in incremental commits, the PR shows those, as well as previous ones. However, if you rewrite commit history by force-pushing amended commit(s), the previous ones are no longer associated with the PR and only the latest commit will have status.

Compare:

forcepush-2329

to:

incremental-2327

Keeping previous results is useful to see the issues addressed, and also helps us identify flaky tests.

bharatviswa504
bharatviswa504 reviewed Jun 14, 2021
View reviewed changes
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestKeyCreation.java
arg.addLocationInfo(
keyManager.allocateBlock(arg, session.getId(), new ExcludeList()));
}
keyManager.commitKey(arg, session.getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you avoid using these API's for write requests we should use OMKey*Request? (For write requests we don't use any of the code from (Volume/Bucket/Key)Manager classes.

These methods need to be removed, but we have never got to that work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@adoroszlai adoroszlai changed the title Hdds 5315 Add basic unit tests for skipping tokens HDDS-5315. Add basic unit tests for skipping tokens Jun 14, 2021
@adoroszlai
Copy link
Contributor

/pending avoid using KeyManager for write request

github-actions[bot]
github-actions bot requested changes Oct 8, 2021
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

avoid using KeyManager for write request

@kerneltime
Copy link
Contributor Author

Closing this PR for now. Will revisit it once the code clean up is done.

@kerneltime kerneltime closed this Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bharatviswa504 bharatviswa504 bharatviswa504 left review comments

@github-actions github-actions[bot] github-actions[bot] requested changes

Assignees
No one assigned
Labels
pending
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kerneltime @adoroszlai @cxorm @bharatviswa504