Skip to content

HDDS-7286. Clean up ContainerManager#3797

Merged
kaijchen merged 1 commit intoapache:masterfrom
myskov:HDDS-7286_cleanup_ContainerManager
Oct 9, 2022
Merged

HDDS-7286. Clean up ContainerManager#3797
kaijchen merged 1 commit intoapache:masterfrom
myskov:HDDS-7286_cleanup_ContainerManager

Conversation

@myskov
Copy link
Contributor

@myskov myskov commented Oct 4, 2022

What changes were proposed in this pull request?

Clean up of org.apache.hadoop.hdds.scm.container.ContainerManager and its descendants. Removed various code smells such as

  • using deprecated methods
  • missing javadocs
  • using methods annotated VisibleForTesting in production code

What is the link to the Apache JIRA

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

Please replace this section with the link to the Apache JIRA)

How was this patch tested?

Unit tests

@myskov myskov force-pushed the HDDS-7286_cleanup_ContainerManager branch from 3935227 to 75268aa Compare October 4, 2022 20:35
@myskov myskov force-pushed the HDDS-7286_cleanup_ContainerManager branch from 75268aa to 3521b1a Compare October 5, 2022 04:33
Copy link
Contributor

@Galsza Galsza left a comment

Choose a reason for hiding this comment

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

Minor nitpicking, otherwise looking good to me

@Galsza
Copy link
Contributor

Galsza commented Oct 5, 2022

@myskov
Copy link
Contributor Author

myskov commented Oct 5, 2022

@Galsza, I did a force-push to retrigger verifications due to a flaky test. Is it possible to permit contributors to retrigger verifications?

@Galsza
Copy link
Contributor

Galsza commented Oct 5, 2022

I think you either have to ask a committer to restart the pipeline, or add a commit with very small changes. @adoroszlai this is the two best practice, right?

@adoroszlai
Copy link
Contributor

@myskov Committers can retrigger via Github, which allows rerunning only failed jobs. So the best way for contributors is to ping one or more committers. Otherwise, you can retrigger by any further commit, even an empty one:

git commit --allow-empty -m 'trigger new CI check'
git push

@myskov
Copy link
Contributor Author

myskov commented Oct 6, 2022

@Galsza could you merge the PR, please?

@adoroszlai adoroszlai self-requested a review October 6, 2022 13:19
@myskov
Copy link
Contributor Author

myskov commented Oct 7, 2022

@adoroszlai please take a look at the PR

Copy link
Member

@kaijchen kaijchen left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @myskov for the work.

@kaijchen kaijchen merged commit 625e84f into apache:master Oct 9, 2022
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