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

QACI-406 fixed tck websockets #4876

Merged
merged 6 commits into from
Sep 24, 2020
Merged

QACI-406 fixed tck websockets #4876

merged 6 commits into from
Sep 24, 2020

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Sep 7, 2020 •
edited
Loading

Description

Fixes QACI-406 - two TCK tests failed.
Current result:

real    103m6,578s
user    30m29,330s
sys     2m5,713s
Waiting for the domain to stop ..
Command stop-domain executed successfully.
Not passed: 0/745 /home/dmatej/work/repo/git/jakartaeetck-runner/cts_home/jakartaeetck-report/websocket/text/summary.txt
Writing stage file for websocket  into stage_websocket

Important Info

Blockers

Testing

New tests

  • None

Testing Performed

  • TCK tests for websockets - two were failing, now all websockets TCK tests passed
  • Complete Grizzly build, one test is still failing, in older releases failed two
  • Complete Payara build,
  • Payara docker tests
  • Payara Samples - payara-server-managed
  • Payara Samples - payara-micro-managed
  • PAYARA-4176+CUSTCOM-55 tests (I originally tested with it)
  • one local new test is still failing => I will create a new issue.

Testing Environment

Apache Maven 3.6.2 (40f52333136460af0dc0d7232c0dc0bcf0d9e117; 2019-08-27T17:06:16+02:00)
Maven home: /home/dmatej/work/apache-maven-3.6.2
Java version: 1.8.0_265, vendor: Azul Systems, Inc., runtime: /usr/lib/jvm/zulu8.48.0.53-ca-jdk8.0.265-linux_x64/jre
Default locale: cs_CZ, platform encoding: UTF-8
OS name: "linux", version: "5.4.0-45-generic", arch: "amd64", family: "unix"

Documentation

See JIRA issue, links to Confluence will be added

Notes for Reviewers

  • Relates to QACI-409 Logging and debugging jakartaeetck-runner#13
  • This PR fixes ALPN support and passed the TCK/websockets, BUT there is still more work needed. I did not push failing test for a race condition, detected in HTTP2 impl in all older versions I tested, where the HTTP2 support could be enabled.

David Matejcek added 6 commits September 7, 2020 17:45
QACI-406 Changed ordering of JVM options to be same in all domain.xml…
882141a 
… files

- files significantly differ, should be compared and reviewed soon
QACI-406 Using same bootclasspath set in all domain.xml files
1ce43b4
QACI-406 grizzly-npn-bootstrap 1.9.payara-p1 was never used
5939186 
- Added explanation of how it works and what to care about
- removed 1.9.payara-p1 from dependencies
- grizzly-npn-bootstrap is used only for old JDK8 versions, new JDK8 distros
  already contain JSSE implementation supporting ALPN, so no bootstrap is needed
- changed the property so it directly says what dependency it affects
QACI-406 Upgraded Tyrus from 1.15 to 1.17
4bf1130 
- supports TLS1.3
- still loses critical exceptions (server may hang) if Error occurs
  in forked thread
- bugfixes
QACI-406 ServerLogTest enabled again
2622799
QACI-406 Using released version of grizzly 2.4.4.payara-p4
98fb181
@dmatej dmatej self-assigned this Sep 7, 2020
@dmatej dmatej requested review from MattGill98 and MeroRai September 7, 2020 17:13
@dmatej dmatej marked this pull request as ready for review September 7, 2020 17:13
@dmatej dmatej requested a review from MarkWareham September 8, 2020 06:11
@dmatej dmatej added the PR: DO NOT MERGE Don't merge PR until further notice label Sep 11, 2020
dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/gf_template/src/main/resources/config/domain.xml
@@ -430,19 +429,20 @@
<jvm-options>[9|]--add-opens=java.naming/javax.naming.spi=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.rmi/sun.rmi.transport=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.logging/java.util.logging=ALL-UNNAMED</jvm-options>
<jvm-options>-Djava.awt.headless=true</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block.

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/gf_template_web/src/main/resources/config/domain.xml
@@ -200,20 +200,22 @@
<jvm-options>[9|]--add-opens=java.naming/javax.naming.spi=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.rmi/sun.rmi.transport=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.logging/java.util.logging=ALL-UNNAMED</jvm-options>
<jvm-options>-Djava.awt.headless=true</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block.

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/gf_template_web/src/main/resources/config/domain.xml
<jvm-options>-Djdbc.drivers=org.h2.Driver</jvm-options>
<jvm-options>-Djdbc.drivers=org.h2.Driver</jvm-options>
<jvm-options>-Djdk.corba.allowOutputStreamSubclass=true</jvm-options>
<jvm-options>-Djdk.tls.rejectClientInitiatedRenegotiation=true</jvm-options>
Copy link
Contributor Author

@dmatej dmatej Sep 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/production_domain_template/src/main/resources/config/domain.xml
@@ -188,31 +188,31 @@
<jvm-options>-XX:+UseG1GC</jvm-options>
<jvm-options>-XX:+UseStringDeduplication</jvm-options>
<jvm-options>-XX:MaxGCPauseMillis=500</jvm-options>
<jvm-options>-XX:MetaspaceSize=256m</jvm-options>
<jvm-options>-XX:MaxMetaspaceSize=2g</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/production_domain_template/src/main/resources/config/domain.xml
@@ -395,30 +395,30 @@
<jvm-options>-XX:+UseG1GC</jvm-options>
<jvm-options>-XX:+UseStringDeduplication</jvm-options>
<jvm-options>-XX:MaxGCPauseMillis=500</jvm-options>
<jvm-options>-XX:MetaspaceSize=256m</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/production_domain_template_web/src/main/resources/config/domain.xml
@@ -206,36 +206,36 @@
<jvm-options>[9|]--add-opens=java.naming/javax.naming.spi=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.rmi/sun.rmi.transport=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.logging/java.util.logging=ALL-UNNAMED</jvm-options>
<jvm-options>-Xmx2g</jvm-options>
<jvm-options>-Xms2g</jvm-options>
<jvm-options>-XX:+UseG1GC</jvm-options>
<jvm-options>-XX:+UseStringDeduplication</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/admin/production_domain_template_web/src/main/resources/config/domain.xml
@@ -401,40 +401,40 @@
<jvm-options>[9|]--add-opens=java.base/java.util=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.base/sun.nio.ch=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.management/sun.management=ALL-UNNAMED</jvm-options>
<jvm-options>[9|]--add-opens=java.naming/javax.naming.spi=ALL-UNNAMED</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintainability: Only ordering changed in this block

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/extras/embedded/all/src/main/resources/config/domain.xml
<jvm-options>[1.8.0|1.8.0u120]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.6.jar</jvm-options>
<jvm-options>[1.8.0u121|1.8.0u160]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.7.jar</jvm-options>
<jvm-options>[1.8.0u161|1.8.0u190]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.8.jar</jvm-options>
<jvm-options>[1.8.0u191|1.8.0u500]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.8.1.jar</jvm-options>
Copy link
Contributor Author

@dmatej dmatej Sep 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Except ordering here I changed also filter from 1.8.0u500 to 1.8.0u250 just as it is in other domain.xml files and added non-api.

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
appserver/extras/embedded/web/src/main/resources/config/domain.xml
<jvm-options>[1.8.0|1.8.0u120]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.6.jar</jvm-options>
<jvm-options>[1.8.0u121|1.8.0u160]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.7.jar</jvm-options>
<jvm-options>[1.8.0u161|1.8.0u190]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.8.jar</jvm-options>
<jvm-options>[1.8.0u191|1.8.0u500]-Xbootclasspath/p:${com.sun.aas.installRoot}/lib/grizzly-npn-bootstrap-1.8.1.jar</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except ordering here I changed also filter from 1.8.0u500 to 1.8.0u250 just as it is in other domain.xml files and added non-api.

dmatej
dmatej commented Sep 11, 2020
View reviewed changes
nucleus/admin/template/src/main/resources/config/domain.xml
@@ -311,18 +311,19 @@
<diagnostic-service/>
<java-config debug-options="-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=${JAVA_DEBUGGER_PORT}" system-classpath="" classpath-suffix="">
<jvm-options>-server</jvm-options>
<jvm-options>-Djava.awt.headless=true</jvm-options>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just the ordering to be same as in other domain.xml files

@dmatej dmatej removed the PR: DO NOT MERGE Don't merge PR until further notice label Sep 22, 2020
@dmatej dmatej requested a review from MattGill98 September 22, 2020 15:41
MattGill98
MattGill98 approved these changes Sep 23, 2020
View reviewed changes
Copy link
Contributor

@MattGill98 MattGill98 left a comment

Choose a reason for hiding this comment

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

Looks good to me, nice work David!

@MattGill98
Copy link
Contributor

jenkins test please

1 similar comment
@MattGill98
Copy link
Contributor

jenkins test please

@MattGill98
Copy link
Contributor

Jenkins failed because of invalid checksums, which payara/Payara_PatchedProjects#335 should fix

@MattGill98 MattGill98 merged commit 4815472 into payara:master Sep 24, 2020
@dmatej
Copy link
Contributor Author

dmatej commented Sep 27, 2020

I am thinking how this could happen ... seems the problem is with the merge.
Branch A comes with checksum A, branch T comes has checksum T, then merge creates a new version of the file in branch T, but does not generate new sha, only replaces the content of the checksum file with a version coming from A.
Probably until we would use some standard Nexus server we should do this "inspection" periodically after each merge, because we cannot guarantee that merge will not break checksums.

Alterntively we could use some git hook script, but I'm not sure github would execute them.

@dmatej dmatej deleted the QACI-406-fixed-tck-websockets branch September 27, 2020 10:23
aubi pushed a commit to aubi/Payara that referenced this pull request Jan 3, 2022
@MattGill98 @Pandrex247
QACI-406 Merge pull request payara#4876 from dmatej/QACI-406-fixed-tc…
551f19c 
…k-websockets

QACI-406 fixed tck websockets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattGill98 MattGill98 MattGill98 approved these changes

@MeroRai MeroRai Awaiting requested review from MeroRai

@MarkWareham MarkWareham Awaiting requested review from MarkWareham

Assignees

@dmatej dmatej

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dmatej @MattGill98