Skip to content

Don't keep mutable config instance on a field#14475

Merged
ebyhr merged 2 commits intotrinodb:masterfrom
ebyhr:ebi/config-instance
Oct 7, 2022
Merged

Don't keep mutable config instance on a field#14475
ebyhr merged 2 commits intotrinodb:masterfrom
ebyhr:ebi/config-instance

Conversation

@ebyhr
Copy link
Copy Markdown
Member

@ebyhr ebyhr commented Oct 5, 2022

Description

Don't keep mutable config instance on a field

Release notes

(x) This is not user-visible or docs only and no release notes are required.

@cla-bot cla-bot bot added the cla-signed label Oct 5, 2022
@ebyhr ebyhr added maintenance Project maintenance task no-release-notes This pull request does not require release notes entry labels Oct 5, 2022
Copy link
Copy Markdown
Contributor

@ksobolew ksobolew left a comment

Choose a reason for hiding this comment

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

❤️ 🙌

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: requireNonNull is not needed in Java 15+ when we immediately dereference the variable, because the NPE message will be as good or better. @findepi removed such calls en masse recently.

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Oct 5, 2022

Choose a reason for hiding this comment

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

Is this gap on purpose?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No. Thanks for catching that. Removed the empty lines.

@ebyhr
Copy link
Copy Markdown
Member Author

ebyhr commented Oct 6, 2022

CI hit #14487

@ebyhr ebyhr requested review from findepi and hashhar October 6, 2022 00:34
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

redundant

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

redundant, see #13940

@ebyhr ebyhr force-pushed the ebi/config-instance branch from 6baa316 to 21c6004 Compare October 6, 2022 21:04
@ebyhr
Copy link
Copy Markdown
Member Author

ebyhr commented Oct 7, 2022

Compatibility failed due to lack of the latest docker image. https://trinodb.slack.com/archives/CFP480UKX/p1665108934638869

@ebyhr ebyhr merged commit e9c77dd into trinodb:master Oct 7, 2022
@ebyhr ebyhr deleted the ebi/config-instance branch October 7, 2022 13:40
@github-actions github-actions bot added this to the 400 milestone Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed maintenance Project maintenance task no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

3 participants