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

CB-16465 Lazy load safe toString() methods #12485

Closed
wants to merge 1 commit into from
Closed

CB-16465 Lazy load safe toString() methods #12485

wants to merge 1 commit into from

Conversation

Bajzathd
Copy link
Contributor

@Bajzathd Bajzathd commented Mar 23, 2022 •
edited

Added tests for Entity classes to make sure they implement toString() correctly.

@Bajzathd Bajzathd force-pushed the CB-16465 branch 4 times, most recently from 76640f8 to 92200ab Compare March 28, 2022 09:39
@doktoric
Copy link
Contributor

it would be nice to see unit tests just to make sure we dont break this

@Bajzathd
Copy link
Contributor Author

it would be nice to see unit tests just to make sure we dont break this

I have added EntityToStringTest for this purpose, do we need other tests?

@Bajzathd Bajzathd force-pushed the CB-16465 branch 2 times, most recently from 935836a to 4e27d15 Compare March 29, 2022 09:35
@Bajzathd Bajzathd requested a review from doktoric March 31, 2022 11:26
@Bajzathd Bajzathd requested review from a team as code owners April 11, 2022 12:25
@Bajzathd
Copy link
Contributor Author

I have realized that my initial test only covered the entities included in the core service, so I have moved the original unit test to the common module, and extended it in each of our services (FreeipaEntityToStringTest, FreeipaEntityToStringTest, etc)

@Bajzathd Bajzathd force-pushed the CB-16465 branch 2 times, most recently from a2832ed to 60694a3 Compare April 11, 2022 13:09
@lnardai lnardai self-requested a review April 12, 2022 08:20
@Bajzathd Bajzathd force-pushed the CB-16465 branch 2 times, most recently from f22ff17 to 92a55fb Compare April 14, 2022 08:49
@lnardai
Copy link
Contributor

lnardai commented Apr 19, 2022 •
edited

I'm a little concerned about the extended pressure on the logging system. Might worth to check how much more we log during mock tests. Sadly I don't see the results of of the IT test anymore.

@Bajzathd
Copy link
Contributor Author

I do not think that it will have a huge impact, and it is better to log at least the crns of domain objects. But I will compare the IT log size to be sure.

@Bajzathd
Copy link
Contributor Author

@lnardai IT log sizes:
[integcb-logs.tar.gz] 175.02 MB
[test.out] 69.18 MB
These are pretty similar to previous runs' file sizes

@Bajzathd Bajzathd requested a review from lnardai May 4, 2022 09:05
lajosrodek
lajosrodek suggested changes May 12, 2022
View reviewed changes
Copy link
Contributor

@lajosrodek lajosrodek left a comment

Choose a reason for hiding this comment

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

Nice work! A few minor comments.

common/src/main/java/com/sequenceiq/cloudbreak/util/DatabaseUtil.java Show resolved Hide resolved
common/src/main/java/com/sequenceiq/cloudbreak/util/DatabaseUtil.java Outdated Show resolved Hide resolved
common/src/test/java/com/sequenceiq/cloudbreak/AbstractEntityToStringTest.java Outdated Show resolved Hide resolved
common/src/test/java/com/sequenceiq/cloudbreak/AbstractEntityToStringTest.java Outdated Show resolved Hide resolved
common/src/test/java/com/sequenceiq/cloudbreak/AbstractEntityToStringTest.java Outdated Show resolved Hide resolved
redbeams/src/main/java/com/sequenceiq/redbeams/domain/stack/Network.java Show resolved Hide resolved
redbeams/src/main/java/com/sequenceiq/redbeams/domain/stack/Network.java Show resolved Hide resolved
redbeams/src/main/java/com/sequenceiq/redbeams/domain/stack/SecurityGroup.java Outdated Show resolved Hide resolved
redbeams/src/main/java/com/sequenceiq/redbeams/domain/stack/SecurityGroup.java Outdated Show resolved Hide resolved
workspace/src/main/java/com/sequenceiq/cloudbreak/workspace/model/User.java Outdated Show resolved Hide resolved
@lajosrodek
Copy link
Contributor

@Bajzathd A general note: I suggest creating a wiki and sending a msg to Slack regarding the usage patterns & expectations introduced here.

@Bajzathd
CB-16465 Lazy load safe toString() methods
4026fae 
Added tests for Entity classes to make sure they implement toString() correctly.
@Bajzathd
Copy link
Contributor Author

Thank you @lajosrodek for the review! I have answered a question and fixed the others. The wiki is a good idea, I will create it shortly.

@Bajzathd
Copy link
Contributor Author

Wiki page is created: https://cloudera.atlassian.net/wiki/spaces/ENG/pages/9980936961/Guideline+for+entity+toString+implementation

@lnardai
Copy link
Contributor

lnardai commented Aug 15, 2022

Should we close this or you plan to resolve conflicts? Should we this a draft in meanwhile?

@Bajzathd
Copy link
Contributor Author

I'm closing this

@Bajzathd Bajzathd closed this Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lajosrodek lajosrodek lajosrodek requested changes

@doktoric doktoric Awaiting requested review from doktoric

@lnardai lnardai Awaiting requested review from lnardai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Bajzathd @doktoric @lnardai @lajosrodek