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.

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

LPS-91113 Fix tests. Deploy portlet as method is now taking it to account if portlet is deployed or not. #69619

Closed
wants to merge 4 commits into from
Closed

LPS-91113 Fix tests. Deploy portlet as method is now taking it to account if portlet is deployed or not. #69619

wants to merge 4 commits into from

Conversation

marianoalvarosaiz
Copy link

No description provided.

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering "ci:test:sf" and "ci:test:relevant" for this pull to run Source Formatter and relevant tests.

Comment "ci:test" to run the full PR Tester for this pull.

@marianoalvarosaiz
Copy link
Author

/cc @topolik

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 2 minutes 459 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 8e1001f335e250e3d07443ba44f6618892ab302e

Sender Branch:

Branch Name: LPS-91113
Branch GIT ID: a2b68d54a4becde495cc12fff2025cc506557fcc

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@brianchandotcom
Copy link
Owner

@marianoalvarosaiz see changes in upstream. Thx.

@brianchandotcom
Copy link
Owner

@shuyangzhou fyi

@liferay-continuous-integration
Copy link
Collaborator

❌ ci:test:relevant - 63 out of 81 jobs passed in 2 hours 47 minutes 54 seconds 355 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 8e1001f335e250e3d07443ba44f6618892ab302e

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: ad993e7d480e91b7edfb73fee0c1bc055135b8f2

63 out of 81 jobs PASSED

18 Failed Jobs:

63 Successful Jobs:
For more details click here.

Failures unique to this pull:

  6. ...

For upstream results, click here.

@shuyangzhou
Copy link

@marianoalvarosaiz base on the ticket description, this should be a performance ticket, why is it a security ticket?
And all performance changes need to run through a performance check to be sure it is really doing what you think it is doing.

@slnn please run a regression check against it.

@slnn
Copy link

slnn commented Mar 19, 2019

OK, got it and will update test results when I got them.

@marianoalvarosaiz
Copy link
Author

Hi @shuyangzhou,
I marked it as a security issue because it seems really easy to create a script with a counter making a huge number of calls changing the p_p_id, that will end with lots of queries to a heavy table like PortletPreferences (since the portletId changes and will not be cached) hence causing a DB DoS
Anyway I've marked it as a Performance issue also.
Kind regards.

/cc @topolik

@slnn
Copy link

slnn commented Mar 19, 2019 •
edited

Hi @shuyangzhou,

Here are test results:
Login:

Portal Version Git Id Session Count Mean Time of Login Mean Time of Logout Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 15850 262 64.1 - 0,0,0
PR-69619 - 15968 283 71.7 0.7444794953 0,0,0

WebContent:

Portal Version Git Id Session Count Mean Time of Login Mean Time of View Page Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 21128 402 151   1,1,1
PR-69619 - 20637 393 149 -2.323930329 1,1,1

MessageBoard:

Portal Version Git Id Session Count Mean Time of Login Mean Time of Add MB Thread Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 7597 221 374   18,21,23
PR-69619 - 7577 118 285 -0.2632618139 17,20,23

Blog:

Portal Version Git Id Session Count Mean Time of Login Mean Time of Add Blog Entry Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 5219 67 283   3,4,4
PR-69619 - 5198 75.6 296 -0.4023759341 4,4,4

Document Library:

Portal Version Git Id Session Count Mean Time of Login Mean Time of Add DL File Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 8680 568 882   10,10,10
PR-69619 - 8742 315 629 0.7142857143 10,10,10

Wiki:

Portal Version Git Id Session Count Mean Time of Login Mean Time of Add Wiki Page Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 5596 356 562   9,10,9
PR-69619 - 5435 397 591 -2.877055039 10,9,9

AssetPublisher:

Portal Version Git Id Session Count Mean Time of Login Mean Time of VIew Page with Filter Session Count Changes
BaseLine 8e1001f 9172 143 158  
PR-69619 - 9113 132 154 -0.643262102

Wiki

Portal Version Git Id Session Count Mean Time of Login Mean Time of VIew Page with Filter Session Count Changes DB CPU Usage(%)
BaseLine 8e1001f 9172 143 158   1,1,1
PR-69619 - 9113 132 154 -0.643262102 1,1,1

@shuyangzhou
Copy link

@slnn since this is mainly for dB. Please modify the result comment to include dB side CPU usage diff.

@marianoalvarosaiz as you can see, this is not improve anything, at least from the overall performance's point of view. And webcontent and wiki seem to be a lost, although this could still be within noise range. Once lily attaches dB CPU usages, we can have a better understanding. The point is, there should be no guess work when doing performance changes. Common sense usually fails here, only the changes backed by test results can be trusted.

@slnn
Copy link

slnn commented Mar 20, 2019

Hi @shuyangzhou,

Update the DB CPU usage be finished.

@shuyangzhou
Copy link

@marianoalvarosaiz there is no real noticeable diff in general. All diff are within noise range. Although they are towarding to be slightly loss.
In this case there is no immediate pressure of making further changes. But it will help the future if you can create a test to secure your expected logic. And when we get more bandwidth we can revisit the issue using your test as acceptance test to make more performance improvements if that is ever needed.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
ci:test:relevant - failure ci:test:sf - success
Projects
None yet
Milestone
No milestone
6 participants
@marianoalvarosaiz @liferay-continuous-integration @brianchandotcom @shuyangzhou @slnn @topolik