Project

General

Profile

Support #5567

move FWD to Java 11

Added by Greg Shah over 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:

hotel_chui_java_11.diff Magnifier (6.37 KB) Tomasz Domin, 04/19/2022 11:05 AM

hotel_gui_java_11.patch Magnifier - 20220509 hotel gui patch (2.9 KB) Tomasz Domin, 05/09/2022 12:55 PM

hotel_chui_java_11.diff Magnifier - 20220509 hotel chui patch (6.37 KB) Tomasz Domin, 05/09/2022 12:55 PM

5567-1.diff Magnifier (1.71 KB) Tomasz Domin, 07/21/2022 02:07 PM

5567-2.diff Magnifier - JDK11 compatibility patch (1.71 KB) Eugenie Lyzenko, 07/21/2022 03:41 PM


Related issues

Related to Database - Feature #5491: custom locale/collation implementations WIP
Related to Runtime Infrastructure - Feature #6692: move FWD to Java 17 Internal Test

History

#1 Updated by Greg Shah over 2 years ago

  • Related to Feature #5491: custom locale/collation implementations added

#2 Updated by Greg Shah over 2 years ago

Make the changes needed to move everything (conversion, data import, runtime...) to Java 11.

Java 9 Release Notes
Java 10 Release Notes
Java 11 Release Notes

  1. Upgrade all of our dependencies to levels that support Java 11.
  2. Make changes to our Java source code and build.
    • Dependencies on Java EE features like CORBA have been removed from Java SE. This means usage of javax.activation, javax.mi, javax.annotation and other packages are now eliminated. The fix is to add external jars that provide these packages or remove the usage.
    • Internal APIs like com.sun.* and sun.* are gone or substantially reduced (e.g. sun.misc.Unsafe). We need to eliminate these dependencies/replace them with another approach.
    • Some deprecated APIs have been removed, as has JavaFX. Any dependencies should be removed or replaced.
    • Any assumption that the application class loader is an instance of URLClassLoader is no longer valid. This means we may need to write some helper methods to make up for the difference.
    • If we use _ (by itself) as a variable or method name, it will no longer work starting in Java 9.
    • Move from javah (gone in Java 10) to javac -h.
    • Lucida fonts have been removed.
  3. There will be some effort needed for the text collation SPI which is loadable from the classpath starting in Java 9 (and the traditional Java extension mechanism is no longer there). See #5491-110.

There may be more issues as well, especially to get everything to compile/build properly. The above is just some items that I found from reviewing the release notes and reading some articles.

#3 Updated by Greg Shah about 2 years ago

  • Assignee set to Tomasz Domin

#4 Updated by Tomasz Domin about 2 years ago

  • Status changed from New to WIP

First step - prepare a plan.
Need to review all the code and all tools for problems with JDK11

#5 Updated by Greg Shah about 2 years ago

Via email, from Tomasz:

Created branch 5567a, started porting with fixing compilation bugs. I've considered upgrading gradle, but for now I'll skip it unless necessary.

Please make sure to rebase 5567a from the latest 3821c. The differences from trunk are too great. 3821c is essentially the trunk at this point. You'll have to try to keep up with the changes by rebasing frequently. That will minimize the effort later.

#6 Updated by Tomasz Domin about 2 years ago

Greg Shah wrote:

Via email, from Tomasz:
Please make sure to rebase 5567a from the latest 3821c. The differences from trunk are too great. 3821c is essentially the trunk at this point. You'll have to try to keep up with the changes by rebasing frequently. That will minimize the effort later.

Actually I started with 5567a branched from trunk and planned having 5567b as a branch from 3821c, as I didn't know if there are any projects based on clean trunk.
But I will merge 3821c into 5567a as requested.

#7 Updated by Greg Shah about 2 years ago

as I didn't know if there are any projects based on clean trunk.

All projects are dependent upon 3821c at this point.

#8 Updated by Tomasz Domin about 2 years ago

  • % Done changed from 0 to 10

Till now worked on trunk branch, as I was already working on it:

Added dependencies missing in java 11:
fwdCCS group: 'javax.xml.bind', name: 'jaxb-api', version: '2.3.0'
fwdCCS group: 'jakarta.xml.ws', name: 'jakarta.xml.ws-api', version: '2.3.3'

Updated GWT to 2.9.0
Changed ant-compile target to generate jni headers
Fixed native build

gradle all works without errors.

Next - rebase to 3821c

#9 Updated by Tomasz Domin about 2 years ago

Attached for the reference only - patch against trunk branch

branch 5567a:
Rebased to 3821c, pushed to 5567a, commit 13778

In order to check actual runtime (fonts, spi etc) I need a reference project.
As is still having issues I will try to check hotel_chui and hotel_ui apps.

#10 Updated by Tomasz Domin about 2 years ago

  • % Done changed from 10 to 0

Built hotel_gui, but it does not work correctly.
Most of the scripts for hotel_*ui are based on system-wide java installation (do I need to switch alternative), maybe script should be modified to make use of JAVA_HOME env ?

#11 Updated by Tomasz Domin about 2 years ago

Client Web GUI app works except there is a problem with font sizes.
The reason is modularization and ClassLoader.class.getResourceAsStream does not work in Java 11 as it did in Java 8, so font-metrics is not read correctly.
Funny thing - now there is issue like #5367, this may suggest #5367 is cause by a bug in font adaptation to the metrics.

#12 Updated by Tomasz Domin about 2 years ago

Client Web GUI - both embedded and Virtual Desktop are now working fine
But I cannot make Swing GUI work - there is only yellow rectangle on screen, the application starts normally and even allows blind log-in. But nothing is visible on screen. No Exception in logs either, like this case was not predicted. Seems like rendering surface is not created.

#13 Updated by Tomasz Domin about 2 years ago

  • % Done changed from 0 to 40

Tomasz Domin wrote:

But I cannot make Swing GUI work - there is only yellow rectangle on screen, the application starts normally and even allows blind log-in.

Well, the root cause was not Swing GUI app but unattended upgrade process that broke display libraries. As I only restart my workstation weekly some session libraries were replaced during unattended upgrades which made java 11 Swing not working correctly (java 8 worked fine). After restarting workstation everything went back to normal. So I unintentionally had to learn the architecture of Swing GUI app :).

Now Swing GUI is working with no issues, committed changes to 5567a.
Started to test ChUI, it builds/compiles fine, but it seems after recent changes in templates its not building anymore not with Java11/5567a nor with java8/3821c, tried to update configuration but did not have time to fix it.

#14 Updated by Tomasz Domin about 2 years ago

Rebased to the latest p2j 3821c (13792)

Managed to build and test hotel_chui - no issues found.
As I dont have a branch for hotel_chui attached a patch to make it build with the latest p2j/5567a

#15 Updated by Tomasz Domin about 2 years ago

  • % Done changed from 40 to 50

Today I've mainly worked on changes to newInstance and datatypes conversion, static accessors
Unfortunately antlr generated code will produce warnings in Java 11 until we migrate to newer version (which is a separate huge tasks probably).
branch/revision: 5567a/13796

#16 Updated by Greg Shah about 2 years ago

Unfortunately antlr generated code will produce warnings in Java 11 until we migrate to newer version (which is a separate huge tasks probably).

See #1757. I've reviewed the options and it is a substantial project. It is also not clear that it is a good idea. The ANTLR v4 is nicer from a development perspective but it drops support for some very important features which we use heavily in our current parsers.

It is more likely that we will modify the ANTLR 2.7.7 to generate code that doesn't have issues. What are the problems in the generated code?

#17 Updated by Tomasz Domin about 2 years ago

Greg Shah wrote:

Unfortunately antlr generated code will produce warnings in Java 11 until we migrate to newer version (which is a separate huge tasks probably).

See #1757. I've reviewed the options and it is a substantial project. It is also not clear that it is a good idea. The ANTLR v4 is nicer from a development perspective but it drops support for some very important features which we use heavily in our current parsers.

It is more likely that we will modify the ANTLR 2.7.7 to generate code that doesn't have issues. What are the problems in the generated code?

These are mostly compile warnings, not a big deal. I cannot tell much about runtime and conversion yet.

#18 Updated by Tomasz Domin about 2 years ago

Today I refactored code to make use of ReflectiveOperationException on .newInstance (instead of 4 other exceptions), merged #6208 into #5567, commited
After running hotel_cli found it does not work, so I need to find the reason.

#19 Updated by Greg Shah about 2 years ago

Tomasz Domin wrote:

Greg Shah wrote:

Unfortunately antlr generated code will produce warnings in Java 11 until we migrate to newer version (which is a separate huge tasks probably).

See #1757. I've reviewed the options and it is a substantial project. It is also not clear that it is a good idea. The ANTLR v4 is nicer from a development perspective but it drops support for some very important features which we use heavily in our current parsers.

It is more likely that we will modify the ANTLR 2.7.7 to generate code that doesn't have issues. What are the problems in the generated code?

These are mostly compile warnings, not a big deal. I cannot tell much about runtime and conversion yet.

Please show an example.

#20 Updated by Tomasz Domin about 2 years ago

Greg Shah wrote:

Please show an example.

antlr warnings are related with objects representing numbers instantiation:

[ant:javac] /home/tjd/projects/p2j_5567a/src/com/goldencode/p2j/e4gl/E4GLLexer.java:218: warning: [deprecation] Integer(int) in Integer has been deprecated
[ant:javac]     literals.put(new ANTLRHashString("server", this), new Integer(14));
...
[ant:javac] /home/tjd/projects/p2j_5567a/src/com/goldencode/p2j/persist/hql/HQLParser.java:1754: warning: [deprecation] Long(long) in Long has been deprecated
[ant:javac]             s_AST.putAnnotation("index", new Long(nextSubstitutionIndex++));
...
[ant:javac] /home/tjd/projects/p2j_5567a/src/com/goldencode/p2j/persist/orm/FQLParser.java:2375: warning: [deprecation] Long(long) in Long has been deprecated
[ant:javac]             s_AST.putAnnotation("index", new Long(nextSubstitutionIndex++));
...
[ant:javac] /home/tjd/projects/p2j_5567a/src/com/goldencode/p2j/preproc/TextLexer.java:188: warning: [deprecation] Integer(int) in Integer has been deprecated
[ant:javac]     literals.put(new ANTLRHashString("&endif", this), new Integer(24));
...

#21 Updated by Tomasz Domin about 2 years ago

Rebased to latest 3821c and still testing

#22 Updated by Greg Shah about 2 years ago

antlr warnings are related with objects representing numbers instantiation:

Let's just modify ANTLR 2.7.7 to fix this code gen issue.

#23 Updated by Tomasz Domin almost 2 years ago

Today:
  • Rebased to latest (fixed) version of 3821c
  • Fixed more warnings - committed today: 5567a/13834
  • Started to work on ANTLR patching to produce Java 11 compliant code.

Regarding patching -what is the prefered way ?
Can I download ANTLR 2.7.7 distribution and patch it in build.xml script in "prepare"/"ant-prepare" task ?

#24 Updated by Greg Shah almost 2 years ago

I don't think we need to be dynamic. Download it and manually modify the code. We will post the customized version in our artifacts repository on proj.goldencode.com and pull the customized ANTLR down via gradle.

#25 Updated by Tomasz Domin almost 2 years ago

Greg Shah wrote:

I don't think we need to be dynamic. Download it and manually modify the code. We will post the customized version in our artifacts repository on proj.goldencode.com and pull the customized ANTLR down via gradle.

Actually I am using <replace> tasks to replace string in generated java file, as there is only case that generates warnings like s/new Integer(/Integer.valueOf(
Most of warnings are fixed, there are two places compiler complains:

/home/tjd/projects/p2j_5567a/src/com/goldencode/p2j/persist/TemporaryBuffer.java:8652: warning: [deprecation] isAccessible() in AccessibleObject has been deprecated
[ant:javac]          boolean accessible = method.isAccessible();

I've tried to change it to canAccess, but it fails in that case, need to check more

[ant:javac] /home/tjd/projects/p2j_5567a/src/com/goldencode/p2j/uast/LexerDumpFilter.java:277: warning: [deprecation] finalize() in Object has been deprecated
[ant:javac]    protected void finalize()

Basically java runtime 9+ does complains about direct usage of finalization - need to check it as well, will try to find use case.

#26 Updated by Greg Shah almost 2 years ago

I don't think we need to be dynamic. Download it and manually modify the code. We will post the customized version in our artifacts repository on proj.goldencode.com and pull the customized ANTLR down via gradle.

Actually I am using <replace> tasks to replace string in generated java file, as there is only case that generates warnings like s/new Integer(/Integer.valueOf(

Does this cause problems with building directly from an IDE? I build from the command line myself, but I know many developers want to do this in a fully integrated fashion.

#27 Updated by Tomasz Domin almost 2 years ago

Greg Shah wrote:

Actually I am using <replace> tasks to replace string in generated java file, as there is only case that generates warnings like s/new Integer(/Integer.valueOf(

Does this cause problems with building directly from an IDE? I build from the command line myself, but I know many developers want to do this in a fully integrated fashion.

This could cause minor problems (generated code being a bit different) in case of using any IDE based antlr tool, but since ANTLR 2.7.7 is old I don't think any IDE supports it in current releases, which means parser generation must be run manually anyway. It should not generate any problems in case of using any ant/gradle based IDE builders as needed tasks would be executed on build just after java classes generation.

I build release from command line, but I edit code with Eclipse IDE, which just needs running antlr target manually before starting any kind of editing.

#28 Updated by Tomasz Domin almost 2 years ago

  • % Done changed from 50 to 70

Copied from #6208#note-25 as it matches here better:

My understanding of this task is migrating codebase and toolset to Java 11. I took the opportunity to fix minor upgrade issues and make it more in-line with the newer JVMs. My goal was also to remove Java 11 compiler warnings, so I took general steps like:
  • make use of Long/Integer/Character.valueOf instead of new Long/Integer/Character which would save CPU and heap (implemented by search&replace)
  • to do a bit of refactoring of related with use of java.reflection - switch from Class.newInstance() to Class.getDeclaredConstructor().newInstance() and instead of using InvocationTargetException or InstantiationException pair I carefully switched to a bit more generic ReflectiveOperationException when possible as Java 11 can generate 4 different exceptions on object instantiation instead of 2 in Java 8

Regarding review please fine inline comments below:

Ovidiu Maxiniuc wrote:

Review of r13835/5567a (branched from r13827/3821a)
  • general: each file's header must be updated with a new historical entry. The (c) year must also be updated.

This supposed to be the last step in order to minimize problems with rebasing, as I need to keep Java 11 branch in parallel with 3821c
Fixed in 5567a/r13848

  • there are occurrences of hard TAB (0x09) (build.xml). These must be replaced with 3 spaces (0x20);

Fixed

  • there are some imports of java.lang.reflect.InvocationTargetException. The preferred way is using bulk:
    [...]Use the single class variant only in case there are conflicts.

Fixed

  • P2JLocaleHelper.java: constructs like:
    [...]
    should be rewritten to take advantage of the autoboxing and vararg parameter calls (already available in Java 5), no need to use the additional objects anymore:
    [...]There are other similar occurrences like XmlFilePlugin:848 and 1035, Mapper:589-614, ProxyAssembler:593, EscapeControlCharsReader:114 and 178, DirectoryService, RemapTestDriver1, Attribute:535-541, RamRemapper, ShadowRemapper, StreamDaemon, hql.g, fql.g, SecurityAdmin, and many others. Generally, instead of <JavaWrapper>.valueOf(javaScalar), use the autoboxing feature. In some rare cases (VectorGraphicsHelper:248-249, Tracer:) a cast to desired type can be necessary;

This is a bit beyond the scope of this task, as I am trying to minimize refactoring so merging with 3821c would be possible at all :)

  • XmlAst: 107-140, 692: use the auto boxing. Special case here since the values are processed below in pair, it seems logical to add them to map on two columns so that the reader can easily understand the logic of the code, like this:
    [...]

This is a bit beyond the scope of this task. Trying to minimize refactoring.

  • AstManagerPlugin:396, BaseDataType:1144, DataObjectFactory:160: the list of exceptions should be split with one item per line if too long (110 chars). Please column align the | in other places, too;

Rechecked formatting.

  • LdapMapGen:605: nice addition of generics. Use the diamond notation <> to simplify the code and let the compiler infer the types for constructor;

There are a lot of warnings (ca. 7500) about references to generic type. Changing all of this is beyond of scope of this task, as it needs to be introduced gradually.

  • BinaryData:563: what about rewriting the line as: int result = Byte.compare(value1[i], value2[i]); ?

Corrected.

  • SecondLevelCacheStatistics, Statistics, QueryStatistics: The addition of the history and copyright header is correct, although I am not sure about the life span of this file. Just move them above the package declaration and add back the class-level javadoc and set the copyright year to "2020-2022";

Fixed. I will change #6208 as well.

  • JsonObject:1602, StreamConnector:232, FontTable:1777-1779: I think these lines can be safely removed;

Removed.

  • LexerDumpFilter:289: why not calling super.finalize(); ? It is a good practice. Even if momentarily it will just call the empty method of Object, its super could be altered to implement this method, too. Not calling the super method could cause the resources of the super class to remained un-freed (is that a word?);

Basically using finalize seems deprecated, as there is no guarantee that it would be ever called and can cause additional issues, there are other ways (explicit and implicit) ways of management of object resources.
Additionally it will be removed in future see: https://openjdk.java.net/jeps/421
I don't think this is that important in this case, as LexerDumpFilter seems to be used during code conversion only, so no long-running runtime issues could occur.
I'll suppress the warning.

  • GuiWebDriver:1560-1582: are the new masks correct? They seem to have different values though (BUTTON1_MASK=16 but BUTTON1_DOWN_MASK=1024);

I tried to migrate from deprecated in Java 11 MouseEvent.getModifiers to MouseEvent.getModifiersEx, so I've switched all masks references to a new method - it needs testing to make sure this replacement works in practice.

  • the GC code guidelines state that the operands and the operators are separated by a space (DatabaseDumpChecker:215, 217). Also one space is used between a statement keyword (if, while, try) and the opening parenthesis (FileDialogGuiImpl:1519, 2889, NormalizePath:150).

Corrected.

300+ files! Phew!

Now I will try to keep it in sync with 3821c :)

#29 Updated by Ovidiu Maxiniuc almost 2 years ago

Tomasz Domin wrote:

My understanding of this task is migrating codebase and toolset to Java 11. I took the opportunity to fix minor upgrade issues and make it more in-line with the newer JVMs. My goal was also to remove Java 11 compiler warnings, so I took general steps like:
  • make use of Long/Integer/Character.valueOf instead of new Long/Integer/Character which would save CPU and heap (implemented by search&replace)

I think using the autoboxing is more readable and possibly more efficient. In old code we were forced to write something like:

list.add(new Integer(0));
Blindly replacing the constructor with the static method may be an improvement but the code looks awful:
list.add(Integer.valueOf(0));
while using the autoboxing is much more simple and expressive:
list.add(0);

  • P2JLocaleHelper.java: constructs like:
    [...]
    should be rewritten to take advantage of the autoboxing and vararg parameter calls (already available in Java 5), no need to use the additional objects anymore:
    [...]There are other similar occurrences like XmlFilePlugin:848 and 1035, Mapper:589-614, ProxyAssembler:593, EscapeControlCharsReader:114 and 178, DirectoryService, RemapTestDriver1, Attribute:535-541, RamRemapper, ShadowRemapper, StreamDaemon, hql.g, fql.g, SecurityAdmin, and many others. Generally, instead of <JavaWrapper>.valueOf(javaScalar), use the autoboxing feature. In some rare cases (VectorGraphicsHelper:248-249, Tracer:) a cast to desired type can be necessary;

This is a bit beyond the scope of this task, as I am trying to minimize refactoring so merging with 3821c would be possible at all :)

Not as much as you think. The majority of the cases falls in the pattern I described above. There are a few cases in these files where it leads to creating a intermediary wrapper variable which can be eliminated. No actual functional changes in the code.

  • XmlAst: 107-140, 692: use the auto boxing. Special case here since the values are processed below in pair, it seems logical to add them to map on two columns so that the reader can easily understand the logic of the code, like this:
    [...]

This is a bit beyond the scope of this task. Trying to minimize refactoring.

FWD is a project actively developing for many year. Inherently, a people change. Reformatting is not really refactoring but, in this case, I think it makes the code much more readable for somebody new. That why I encouraged you to do it.

  • LdapMapGen:605: nice addition of generics. Use the diamond notation <> to simplify the code and let the compiler infer the types for constructor;

There are a lot of warnings (ca. 7500) about references to generic type. Changing all of this is beyond of scope of this task, as it needs to be introduced gradually.

You altered the java 1.4 code

Hashtable env = new Hashtable();
to Java 5+:
Hashtable<String,String> env = new Hashtable<String, String>();
Java 7+ allows the code to be rewritten in a simpler form:
Hashtable<String,String> env = new Hashtable<>();

  • LexerDumpFilter:289: why not calling super.finalize(); ? It is a good practice. Even if momentarily it will just call the empty method of Object, its super could be altered to implement this method, too. Not calling the super method could cause the resources of the super class to remained un-freed (is that a word?);

Basically using finalize seems deprecated, as there is no guarantee that it would be ever called and can cause additional issues, there are other ways (explicit and implicit) ways of management of object resources.
Additionally it will be removed in future see: https://openjdk.java.net/jeps/421
I don't think this is that important in this case, as LexerDumpFilter seems to be used during code conversion only, so no long-running runtime issues could occur.
I'll suppress the warning.

I was not aware of fact that this method becomes deprecated. It seems like a good thing to me. In this particular case, this is a probably a bug. If I am correct, this is the cause for some of .lexer file artifacts were broken (empty). I learned we will drop them in a near future anyway, so suppressing the warning will not do any harm.

  • GuiWebDriver:1560-1582: are the new masks correct? They seem to have different values though (BUTTON1_MASK=16 but BUTTON1_DOWN_MASK=1024);

I tried to migrate from deprecated in Java 11 MouseEvent.getModifiers to MouseEvent.getModifiersEx, so I've switched all masks references to a new method - it needs testing to make sure this replacement works in practice.

These are the riskier changes in all the branch. You should do a smoke test in this area before merging it to parent branch.

#30 Updated by Tomasz Domin almost 2 years ago

Ovidiu Maxiniuc wrote:

  • make use of Long/Integer/Character.valueOf instead of new Long/Integer/Character which would save CPU and heap (implemented by search&replace)

I think using the autoboxing is more readable and possibly more efficient. In old code we were forced to write something like:[...]Blindly replacing the constructor with the static method may be an improvement but the code looks awful:[...]while using the autoboxing is much more simple and expressive:[...]

I fully agree it does not look good in some cases. If autoboxing is more efficient ;), well I would discuss this, it depends on compiler optimizations (which probably are there). I am personally a big fan of doing thing explicitly (e.g. using wrappers), so I can forget autoboxing exists - one thing less to remember sometimes helps. But this is my personal approach.

P2JLocaleHelper.java: constructs like [...] should be rewritten to take advantage of the autoboxing and vararg parameter calls (already available in Java 5), no need to use the additional objects anymore:
[...]There are other similar occurrences like XmlFilePlugin:848 and 1035, Mapper:589-614, ProxyAssembler:593, EscapeControlCharsReader:114 and 178, DirectoryService, RemapTestDriver1, Attribute:535-541, RamRemapper, ShadowRemapper, StreamDaemon, hql.g, fql.g, SecurityAdmin, and many others. Generally, instead of <JavaWrapper>.valueOf(javaScalar), use the autoboxing feature. In some rare cases (VectorGraphicsHelper:248-249, Tracer:) a cast to desired type can be necessary;
  • XmlAst: 107-140, 692: use the auto boxing. Special case here since the values are processed below in pair, it seems logical to add them to map on two columns so that the reader can easily understand the logic of the code, like this:

I made corrections for specified cases, but not gonna do this in general case, unless explicitly being asked to do so, as I still presume this is not in scope of this task

FWD is a project actively developing for many year. Inherently, a people change. Reformatting is not really refactoring but, in this case, I think it makes the code much more readable for somebody new. That why I encouraged you to do it.

Yes, and several things may be improved, but I think it should be done gradually, not in single step.

  • LdapMapGen:605: nice addition of generics. Use the diamond notation <> to simplify the code and let the compiler infer the types for constructor;

You altered the java 1.4 code[...]to Java 5+:[...]Java 7+ allows the code to be rewritten in a simpler form:[...]

This case I did it as a test, implemented a simpler form.

These are the riskier changes in all the branch. You should do a smoke test in this area before merging it to parent branch.

Full testing is part of the plan, and I'd suggest to make it a separate task for dependent projects - like <customer_chui_application> - later on.
I am doing (trying to) full testing for hotel app (chui/swing/web) anyway.

Again - I am trying to minimize changes/risks, as even now I've encountered some issue and have to dig in.
(I am afraid that autoboxing will make it even more risky, but lets try it)

#31 Updated by Tomasz Domin almost 2 years ago

Today I've rebased to latest 3821c and implemented changes/formatings/optimization proposed Ovidiu and started testing with hotel application again.
I've found some regression in Hotel_GUI which seems unrelated with my changes, but with some regression in 3821c as reported in #6149#note-65
Commited 5567a/r13858

#32 Updated by Tomasz Domin almost 2 years ago

Rebased to 3821c/r13853, restarted testing.
Found two more regression cases:
#6149#note-67
#6149#note-68

Other functionality seems to be working fine.

#33 Updated by Greg Shah almost 2 years ago

Are the regressions specific to Java 11 (5567a) or are they in 3821c?

#34 Updated by Tomasz Domin almost 2 years ago

Greg Shah wrote:

Are the regressions specific to Java 11 (5567a) or are they in 3821c?

All are in 3821c.
I've just found them while testing 5567a.

#35 Updated by Ovidiu Maxiniuc almost 2 years ago

Tomasz Domin wrote:

[...] I am trying to minimize changes/risks, as even now I've encountered some issue and have to dig in.
(I am afraid that autoboxing will make it even more risky, but lets try it)

During the weekend I did some research in this area. I was very curious of what are the difference between these three approaches. So I wrote and compile the following code (fragment):

line 13:    List<Integer> list = new ArrayList<>();
line 14:    list.add(0);
line 15:    list.add(new Integer(0));
line 16:    list.add(Integer.valueOf(0));

I compiled it with both Java8 and Java11 and there was practically no difference in the generated bytecode. Then I used javap (the Java Disassembler) to peek at the resulting .class content. Here is how it looks:

line 14:    list.add(0);
        37: aload_1
        38: iconst_0
        39: invokestatic  #47                 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
        42: invokeinterface #53,  2           // InterfaceMethod java/util/List.add:(Ljava/lang/Object;)Z
        47: pop

line 15:    list.add(new Integer(0));
        48: aload_1
        49: new           #48                 // class java/lang/Integer
        52: dup
        53: iconst_0
        54: invokespecial #59                 // Method java/lang/Integer."<init>":(I)V
        57: invokeinterface #53,  2           // InterfaceMethod java/util/List.add:(Ljava/lang/Object;)Z
        62: pop

line 16:    list.add(Integer.valueOf(0));
        63: aload_1
        64: iconst_0
        65: invokestatic  #47                 // Method java/lang/Integer.valueOf:(I)Ljava/lang/Integer;
        68: invokeinterface #53,  2           // InterfaceMethod java/util/List.add:(Ljava/lang/Object;)Z
        73: pop

As you can see, the list.add(0); (autoboxing) and list.add(Integer.valueOf(0)); are identical, both are compiled using the call to Integer.valueOf() static method. The list.add(new Integer(0)); is a bit different.

I was interested whether switching to paradigm imposed by Java11 is faster. Looking at the code (Java11 and Java 8 are the same), we see:

    public static Integer valueOf(int i) {
        if (i >= IntegerCache.low && i <= IntegerCache.high)
            return IntegerCache.cache[i + (-IntegerCache.low)];
        return new Integer(i);
    }

The answer is difficult to be appreciated. If the value is in cache bounds, the object is quickly returned. If not, a new Integer instance will be created which lead to similarly CPU cycles used. It will use some CPU and additional memory from the initialization, but this will probably amortised during the lifetime of the VM.

Conclusion: Integer.valueOf() is the way Java compiler implements autoboxing. Since the bytecode is the same, there is no risk associated with using it compared to the new "explicit" boxing. Also it seems like the constraints imposed by Java11 comes with a slight improvement in resources when the majority of values are in the interned range [-128, 127].

#36 Updated by Tomasz Domin almost 2 years ago

Just a note to myself regarding SPI locale providers.
I though it would be possible to add SPI locale provider without using java.locale.providers or any other command line option.
Its not possible based on https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/spi/LocaleServiceProvider.html :

Applications which require implementations of the locale sensitive services must explicitly specify "SPI" in order for the Java runtime to load them from the classpath.

So I cannot do anything more then adding SPI.jar to classpath and applying change to scripts:

-spi="-Djava.locale.providers=SPI,JRE -Djava.ext.dirs=$(${prog} ${cpath} com.goldencode.util.PrintSystemProp java.ext.dirs):../lib/spi" 
+spi="-Djava.locale.providers=SPI,CLDR,COMPAT" 

#37 Updated by Tomasz Domin almost 2 years ago

Ovidiu Maxiniuc wrote:

During the weekend I did some research in this area. I was very curious of what are the difference between these three approaches. So I wrote and compile the following code > Conclusion: Integer.valueOf() is the way Java compiler implements autoboxing. Since the bytecode is the same, there is no risk associated with using it compared to the new "explicit" boxing. Also it seems like the constraints imposed by Java11 comes with a slight improvement in resources when the majority of values are in the interned range [-128, 127].

Thank you for this research.

Today I've rebased to 3821c/r13853 and committed 5567a/r13865.
I've implemented most of Ovidiu's comments, except replacing .valueOf wrappers with autoboxing.
P2J and hotel_gui/hotel_chui applications run on Java 11.
It needs testing:
  • regression testing to check transactions and internal logic
  • mouse modifiers handlers
  • administration console
  • I havent checked missing Lucida fonts
  • possibly running some manual tests

I think it is ready for review in order to shape future steps or it could wait until regression testing is possible.

How to build it with hotel_gui/hotel_chui apps

#Switch to java 11 (Ubuntu 20.04) systemwide
sudo update-java-alternatives --set java-1.11.0-openjdk-amd64 
cd ~/projects/p2j_5567a
./gradlew all
#hotel GUI
cd ~/projects/hotel_gui
ln -s ../p2j_5567a/ p2j
patch -p1 <hotel_gui_java_11.patch
ant deploy.all

#hotel Chui
cd ~/projects/hotel_chui
ln -s ../p2j_5567a/ p2j
patch -p1 <hotel_chui_java_11.diff
ant deploy.all

#38 Updated by Tomasz Domin almost 2 years ago

Rebased to 3821c rev 13894
Tested hotel_gui and hotel_chui.
Pushed 5567a rev 13906
Probably I would need to rebase once a week until switch FWD to 11 happens

#39 Updated by Greg Shah almost 2 years ago

Hynek: Please review 5567a.

Tomasz:

  • Are there any known issues/regressions and/or other reasons to hold off on merging into 3821c?
  • What will the process be for migrating existing users of 3821c?

#40 Updated by Hynek Cihlar almost 2 years ago

Missing file history entries:
  • AdminServerImpl.java
  • GuiWindowContainer.java
  • GenericWidget
  • comhandle.java

There are many diffs changing aClass.newInstance() to aClass.getDeclaredConstructor().newInstance(). Why is this needed?

procTranslations.get(String.valueOf(tr.line)) in I18nWorker changes the key argument from original Integer to String, this is certainly not correct.

There are many changes that replace concrete reflective exceptions for ReflectiveOperationException. I must say I preferred the original version where it was more obvious right from the code what exceptions can be thrown.

Files with white space changes only:
  • WebClientsManager.java
  • InputDialog.java

JsonObject.java, there is a call to JsonBackend.instance() with the value returned ignored.

In CommonAstSupport.java how does the replaced getConstructors for getDeclaredConstructors relate with JVM 11 compatibility? This is a functional change, is it expected?

MetadataManager.java, DmoMeta.java, BufferFieldImpl.java, BufferImpl.java, DynamicConversionHelper.java, FwdCollatorProvider.java file history entry is misaligned.

TemporaryBuffer.java, the added comment // canAccess(this) does not work deserves a clarification what/why it doesn't work.

I doubt it is useful to to wrap literals in valueOf methods. Considering more code is needed it results in negative gain. Btw. sometimes you wrap the literal sometimes not, which seems to be just random.

AstManagerPlugin.java, GuiWebSocket.java, PaintStructure.java has only history entry, no other change.

FontTable.java, the empty finally block should be removed.

Please remove the commented out code in build.gradle and build.xml.

#41 Updated by Tomasz Domin almost 2 years ago

Greg Shah wrote:

  • Are there any known issues/regressions and/or other reasons to hold off on merging into 3821c?

There are no knows issues.
I think running full regression tests before merging into 3821c would be reasonable.
Or at least some crucial regression selected from Testcases.

  • What will the process be for migrating existing users of 3821c?

High level steps:

  1. Development: migrate any custom (software out of p2j project) platform extensions to java 11.
  2. Install java 11 on build server
  3. Switch build server environment to java 11
  4. Build p2j for java 11
  5. Build app for java 11 with p2j for java 11
  6. Install java 11 on runtime servers
  7. Install app for java 11 on runtime servers
  8. Stop app for java 8, run app for java 11 on runtime servers

Customers need to perform this process on their test environment first.
I think in-place app for java 8 to app for java 11 switchover to java 11 would be risky and testing is required - at least for the first installation.

#42 Updated by Tomasz Domin almost 2 years ago

Thank you Hynek

Hynek Cihlar wrote:

Missing file history entries:
  • AdminServerImpl.java
  • GuiWindowContainer.java
  • GenericWidget
  • comhandle.java

Reverted to old versions, changes are not needed for the task, they were side effect of discussion with Ovidiu about autoboxing.

There are many diffs changing aClass.newInstance() to aClass.getDeclaredConstructor().newInstance(). Why is this needed?

aClass.newInstance() generates compile warning in Java 11. I was trying to clean warnings to remain in line with #6208

procTranslations.get(String.valueOf(tr.line)) in I18nWorker changes the key argument from original Integer to String, this is certainly not correct.

This seems to be a problem with original code, the code below could not work correctly, as tr.line is int, original code:

HashMap<String, List<Translation>> procTranslations = null;
List<Translation> lineTranslations = procTranslations.get(tr.line);

I've changed the code, so now caching should work:

List<Translation> lineTranslations = procTranslations.get(tr.source);

There are many changes that replace concrete reflective exceptions for ReflectiveOperationException. I must say I preferred the original version where it was more obvious right from the code what exceptions can be thrown.

When changing from deprecated aClass.newInstance() to aClass.getDeclaredConstructor().newInstance() (as suggested by Java 11 Class.newInstance() javadoc) I needed to catch more possible exceptions. Thus each try would require two more catch statements. After code analysis I've noted that in most of cases are exception handler is ignoring or rethrowing original exception, so I've used ReflectiveOperationException to generalize in such cases to minimize unneeded handling code. I also found the cases when generalization is not possible as there are different actions on specific exceptions and handled them appropriately - usually by moving generic handlers to ReflectiveOperationException section at the end of try-catch statement.

Files with white space changes only:
  • WebClientsManager.java
  • InputDialog.java

It visible this way due to fact, that #6208 patch has been applied to both 3821c and 5567a (I presume you compare 5567a to the latest 3821c)

AstManagerPlugin.java,

This is due changes I've had reverted in 5567a development. Reverted to version without unneeded history entry.

GuiWebSocket.java, PaintStructure.java has only history entry, no other change.

GuiWebSocket.java and PaintStructure.java have correct headers in 5567a only, need to update 3821c

JsonObject.java, there is a call to JsonBackend.instance() with the value returned ignored.

This is not really related with java 11 migration, it was static access correction. I will revert it back.

In CommonAstSupport.java how does the replaced getConstructors for getDeclaredConstructors relate with JVM 11 compatibility? This is a functional change, is it expected?

Its my mistake, reverted to getConstructors.

MetadataManager.java, DmoMeta.java, BufferFieldImpl.java, BufferImpl.java, DynamicConversionHelper.java, FwdCollatorProvider.java file history entry is misaligned.

Corrected formatting.

TemporaryBuffer.java, the added comment // canAccess(this) does not work deserves a clarification what/why it doesn't work.

Added following comment:

         // tried to migrate to related Method.canAccess(this), but it does not work as expected
         // added @SuppressWarnings("deprecation") to invoke method to ignore
         // warning until a correct migration is implemented

I doubt it is useful to to wrap literals in valueOf methods. Considering more code is needed it results in negative gain. Btw. sometimes you wrap the literal sometimes not, which seems to be just random.

In most cases literals were originally wrapped. I've just changed deprecated new Object(value) to Object.valueOf(value)
Wrapping literals is neutral due to compiler optimizations as Ovidiu verified it #5567#note-35

FontTable.java, the empty finally block should be removed.

Removed.

Please remove the commented out code in build.gradle

Removed

and build.xml.

Actually I've restored commented task ant-schema despite it is not in use anymore, as disabling hibernate is not in scope of Java 11 migration and I think I've removed it

Compiled, tested with hotel gui/chui app
Commit: 5567a rev 13907

#43 Updated by Hynek Cihlar almost 2 years ago

Tomasz Domin wrote:

It visible this way due to fact, that #6208 patch has been applied to both 3821c and 5567a (I presume you compare 5567a to the latest 3821c)

I compared 5567a against 3821c revision 13894, which should have been the base according to #5567-38.

Wrapping literals is neutral due to compiler optimizations as Ovidiu verified it #5567#note-35

Yes. But the wrappers introduce noise in the source code. I.e. you have to mentally process more information than necessary.

Thanks for the clarifications, I'm OK with the other changes.

#44 Updated by Tomasz Domin almost 2 years ago

Rebased to 3821c/13935
Pushed/committed 5567a/13948

#45 Updated by Greg Shah almost 2 years ago

My recommendation is to run ChUI regression testing in both 3821c and 5567a. Because our ChUI regression testing can have false positives (tests that sometimes fail but usually pass on the same level of FWD), we usually run twice (or more) to see what tests are consistently failing. If that doesn't show any regressions, then we probably can merge 5567a into 3821c.

#46 Updated by Tomasz Domin almost 2 years ago

  • Status changed from Review to WIP

#47 Updated by Tomasz Domin almost 2 years ago

I've started regression testing on devsrv01.
I ran conversion regression - there is a single difference in order of fields of a single form (XXXXXXXXtrain.java):

Java 8:

         crewCode.setDbname("xxx");
         crewCode.setTable("crew");
...
         chief.setDbname("xxx");
         chief.setTable("crew");
...
         description.setDbname("xxx");
         description.setTable("crew");
...
         effectiveDate.setDbname("xxx");
         effectiveDate.setTable("crew");
...

Java 11:

         chief.setDbname("xxx");
         chief.setTable("crew");
...
         effectiveDate.setDbname("xxx");
         effectiveDate.setTable("crew");
...
         description.setDbname("xxx");
         description.setTable("crew");
...
         crewCode.setDbname("xxx");
         crewCode.setTable("crew");
...

Looks like conversion succeeded, but there is an issue with custom Java code for customer application and it fails to compile with Java 11 in file XXXXXAccounts.java

  [javac] /home/tjd/testing***:1126: error: unreported exception ReflectiveOperationException; must be caught or declared to be thrown
    [javac]          rc = StandardServer.invoke(2, () -> xxxxx(aKey, aValue, userOrKey, add));
    [javac]                                    ^

It seems to my error, I'll try to fix it.

#48 Updated by Greg Shah almost 2 years ago

The code change is a long-standing bug in how we generate code. It is not a new issue.

For the compile errors, you'll have to prepare changes to apply.

#49 Updated by Tomasz Domin almost 2 years ago

Fixed issue with compilation errors
I had to rebase 5567a to 3821c/13943 as I have updated testing env and I need versions to align
Pushed 5567a/13957
Played a bit with scripts, seems that runtime-regression step needs build step executed separately, which requires clean step when switching from Java 8 to Java 11 and back.
Running tests in background on devsrv01

#50 Updated by Roger Borrello almost 2 years ago

Tomasz Domin wrote:

Played a bit with scripts, seems that runtime-regression step needs build step executed separately, which requires clean step when switching from Java 8 to Java 11 and back.

I'd like to review those changes, if this might affect other projects. Can you make them available?

#51 Updated by Tomasz Domin almost 2 years ago

Roger Borrello wrote:

Tomasz Domin wrote:

Played a bit with scripts, seems that runtime-regression step needs build step executed separately, which requires clean step when switching from Java 8 to Java 11 and back.

I'd like to review those changes, if this might affect other projects. Can you make them available?

Sure, please Java 11 changes are on branch 5567a

Note I havent changed any build/testing scripts, just trying to run them with java 11.
If there are any issue I'll let you know.

#52 Updated by Tomasz Domin almost 2 years ago

Roger Borrello wrote:

I'd like to review those changes, if this might affect other projects. Can you make them available?

For sure I will need to change server startup scrips - SPI configuration is a bit different.
But these are project specific changes

I had to rebase to 3821c/13948, commited 5567a/13962
It seems finally regression tests are running. Generating base with java 8.

#53 Updated by Greg Shah almost 2 years ago

I know we have regressions in the ChUI testing, but I suspect those regressions were already there before 5567a. Do you have enough of a baseline for 3821c/Java 8 that you can compare the 5567a/Java 11 results?

If there is no obvious regression added by 5567a, perhaps the next step is to test this on one of the larger GUI applications. Roger has one that would be a good candidate and he can try it on his system to check the basics. I'd like to merge this into 3821c sooner rather than later, if possible.

#54 Updated by Tomasz Domin almost 2 years ago

Greg Shah wrote:

I know we have regressions in the ChUI testing, but I suspect those regressions were already there before 5567a. Do you have enough of a baseline for 3821c/Java 8 that you can compare the 5567a/Java 11 results?

No, I dont even have Java 8 baseline despite I tried to build it several times, additionally I didnt spent much time on this this week (except for today). I guess the reason of failures was working on latest 3821c but regression scripts were little behind. Tests were freezing due to errors I couldn't fix myself.
Working on this now as I need it for other tasks, hopefully will have it by Monday - as running tests takes a lot of time.

#55 Updated by Tomasz Domin almost 2 years ago

Due to issues with regression testing on latest trunk I still need to hold, reported issue in #5034#note-2358

#56 Updated by Tomasz Domin almost 2 years ago

Main Regression tests were running all night and they are still running, so something is wrong there.
I need to restart and check tests one-by-one as harness does not store intermediate results.

#57 Updated by Tomasz Domin almost 2 years ago

It seems I found an issue in regression tests - runDir is not properly set in tests despite its correctly set in harness commandline:

     [echo] Running harness (smoke test) in dir /home/tjd/testing/xxxxx with runDir=deploy.

Harness commandline

java -Xmx256m -server -cp ../harness/build/lib/harness.jar:. com.goldencode.harness.Harness -h localhost -p 22 -u tjd -v buildPath=/home/tjd/testing/xxxx/ -v runDir=deploy/ -v instanceNum=300 -v instanceRfqNum=310 -v shellPromptText=devsrv01:~$ -v gsoDbName=gso_20090909_staging -v psqlPort=5447 xxxx.xml

Failing tests lines:

 4       CHECK-SCREEN-BUFFER                                                                                                                                                                                                                                                  
wait = true; millis = ʼ180000ʼ; failing screen =                                                                                                                                                                                                                                
tjd@devsrv01:~$ /home/tjd/testing/xxxx/run/client/client.sh -i310                                                                                                                                                                                                              
-bash: /home/tjd/testing/xxxxx/run/client/client.sh: No such file or directory         

I've created symbolic link from run to deploy.
Now I am getting connection refused in client.

#58 Updated by Tomasz Domin almost 2 years ago

As I've changed linked run directory to deploy directory I have to manualy update client.sh script:
Note that client deploy/run directory is hardcoded, i guess this should be parametrized.
from

if [ $script_dir == $HOME/testing/xxxx/deploy/client ]

to
if [ $script_dir == $HOME/testing/xxxx/run/client ]

#59 Updated by Greg Shah almost 2 years ago

The old run/ directory is gone and will not come back. Any remaining references should be found and fixed. It is OK to hard code to deploy/.

#60 Updated by Tomasz Domin almost 2 years ago

Tomasz Domin wrote:

As I've changed linked run directory to deploy directory I have to manualy update client.sh script:
Note that client deploy/run directory is hardcoded, i guess this should be parametrized.
from
[...]
to
[...]

I've rolled back that change, as it works ok for three scenarios which are actually in use:
xxx_single.xml -> this one works
xxx_rctrlc0.xml - not working, execution takes 4 hours
xxx_all1.xml - not fully working, executio2n takes forever

I will try to check what is going on by shortening ctrlc0 and all1 scenarios, as there are no intermediate results for harness and every time I needed to wait till scenarios are fully done.

#61 Updated by Roger Borrello almost 2 years ago

Tomasz Domin wrote:

I will try to check what is going on by shortening ctrlc0 and all1 scenarios, as there are no intermediate results for harness and every time I needed to wait till scenarios are fully done.

See Constantin's note #6213-228

By adding:

<variable name="runDir" type="String" initial="run/" prompt="Enter runDir value" />

It makes majic_single.xml behave. If this is prevalent, I could write a script to modify all the XML, or maybe it's an easy fix in the harness to honor the override.

#62 Updated by Greg Shah almost 2 years ago

You mean this?

<variable name="runDir" type="String" initial="deploy/" prompt="Enter runDir value" />

Go ahead and add it everywhere.

or maybe it's an easy fix in the harness to honor the override

It is not clear to me that there is a bug to fix.

#63 Updated by Roger Borrello almost 2 years ago

Greg Shah wrote:

You mean this?

[...]

Go ahead and add it everywhere.

or maybe it's an easy fix in the harness to honor the override

It is not clear to me that there is a bug to fix.

Well, if you try to override the runDir, it doesn't honor it unless there is a prompt option.

In any case, it's moot because I changed the baseline to have the "runDir=deploy/" using:

grep -Rl "run/" . | xargs sed -i "s/initial=\"run\/\"/initial=\"deploy\/\"/g" 

#64 Updated by Greg Shah almost 2 years ago

Feel free to fix the issue in the harness as well.

#65 Updated by Tomasz Domin almost 2 years ago

For now I have baseline for ctrl-c tests.
I started main regression tests yesterday but they are still running after 12 hours. I am off for next few days so I'll leave it running to see how far it goes.

#66 Updated by Greg Shah almost 2 years ago

The main regression testing should complete in less than 4 hours. Anything more than that suggests many unexpected failures. Each failure may wait up to 10 minutes before it times out, which adds up when there are many failures.

#67 Updated by Constantin Asofiei almost 2 years ago

Greg, there are 2 clamscan processes still running on devsrv01... this usually affects ChUI regression testing.

#68 Updated by Greg Shah almost 2 years ago

Greg, there are 2 clamscan processes still running on devsrv01... this usually affects ChUI regression testing.

True, killed.

#69 Updated by Roger Borrello almost 2 years ago

What is the status of this task? Am I able to do devops image building like Ansible and Docker without including Java 8?

#70 Updated by Greg Shah almost 2 years ago

Not yet. 5567a has not been merged to 3821c.

#71 Updated by Tomasz Domin almost 2 years ago

Rebased to 3821c/14055, patched local version with fixes to #5739, #5737, #6523 and restarted ctrl-c tests with Java 8 to get a newer baseline.

#72 Updated by Tomasz Domin almost 2 years ago

There is issue in sheet:war target with keikai jars and latest JDK 11 - latest javac due to changes in jar/zip file handling stopped accepting archive files that contain files with "." or ".." in it - like ../../../LICENSE.TXT.
Unfortunately all keikai libraries needs to be repacked or updated to 5.10.0 (#6509) to make it work with JDK11.
It works fine when downgraded the libraries to version 5.8.1

#73 Updated by Hynek Cihlar almost 2 years ago

Tomasz Domin wrote:

There is issue in sheet:war target with keikai jars and latest JDK 11 - latest javac due to changes in jar/zip file handling stopped accepting archive files that contain files with "." or ".." in it - like ../../../LICENSE.TXT.
Unfortunately all keikai libraries needs to be repacked or updated to 5.10.0 (#6509) to make it work with JDK11.
It works fine when downgraded the libraries to version 5.8.1

LICENSE.TXT should be placed in the root of the archive. The "ups" are unexpected there. I'll fix this when upgrading to Keikai 5.10.0.

#74 Updated by Greg Shah almost 2 years ago

A customer did some testing on Java 11. They ran branch 6129a which is a modified version of an older 3821c. This 6129a branch was built in Java 8 and has no changes from 5567a. The customer was able to run on Java 11, but found that the performance was 10% to 20% slower than Java 8. This was on OpenJDK but he also tested th Amazon Coretto Java 8/11 with similar results.

He was worried about this issue as the a similar report: https://bugs.openjdk.org/browse/JDK-8252171

#75 Updated by Tomasz Domin almost 2 years ago

Found working regression test and p2j combination (rev 14087).
Rebased to 14087 and pushed to 5567a/*14102*

Baseline:
  • CTRL-C are working fine, so baseline for Java8 is 100% success.
  • main-regression building now on devsrv01, having harness with logging allows detecting bottleneck or failing tests.
To do:
  1. adapt regression tests scripts for java 11 (90% completed)
  2. Run CTRL-C with 5567a - should get 100% success as well
  3. Run main-regression with 5567a - there would be errors, but baseline should be identical to 3821c.

#76 Updated by Greg Shah almost 2 years ago

This is great! When you have equivalent results, please report on the performance between Java 8 and 11.

#77 Updated by Greg Shah almost 2 years ago

Questions for Tomasz:

  1. Is this branch ready for wider testing? If so, I'd like to make a push on testing for this branch to see if we can determine its readiness for merge into 3821c. If it is not ready for wider testing, please note the known issues.
  2. Have Hotel GUI and Hotel ChUI been checked recently?
  3. Are there changes in this 5567a that result in any differences in converted output? If not, perhaps we can execute the same converted code with 3821c rev 14087 and 5567a rev 14102 and the results should be the same (unless there is a regression).

If we try to move ahead today, I would start with this plan:

  • TJD: finish ChUI and CTRL-C regression tests comparision, retest Hotel GUI/Hotel ChUI if not recently tested
  • HC: code review of the changes since the last code review
  • RFB: smoke test for <app_c>
  • EVL: smoke test for <app_m>
  • CA: automated tests for <app_s>

The other issue is the performance cost of Java 11 as seen in a customer app (see my notes #5567-74). A 10-20% penalty for Java 11 is a real problem.

Is there anything else we should consider?

#78 Updated by Constantin Asofiei almost 2 years ago

What's the equivalent for -Djava.ext.dirs=null:../lib/spi in Java 11? I get this error when starting <app_s>:

-Djava.ext.dirs=null:../lib/spi is not supported.  Use -classpath instead.
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

#79 Updated by Tomasz Domin almost 2 years ago

Constantin Asofiei wrote:

What's the equivalent for -Djava.ext.dirs=null:../lib/spi in Java 11? I get this error when starting <app_s>:
[...]

In standard project template server.sh please replace:
spi="-Djava.locale.providers=SPI,JRE -Djava.ext.dirs=$(${prog} ${cpath} com.goldencode.util.PrintSystemProp java.ext.dirs):../lib/spi"
with
spi="-Djava.locale.providers=SPI,CLDR,COMPAT"

#80 Updated by Constantin Asofiei almost 2 years ago

Spawner fails:

2022-07-21 15:07:40.399 ERROR 66965 --- [00000:standard]] com.goldencode.p2j.main.ClientSpawner    : {00000000:0000000B:standard} Failed spawn with exit code: 155

$ cat /var/log/syslog | grep -i spawn
Jul 21 15:07:10 xever spawn[67823]: Error:-101 method:FindClass(NativeSecureConnection) (Success)

This affects both appserver and web clients.

#81 Updated by Roger Borrello almost 2 years ago

Tomasz Domin wrote:

Constantin Asofiei wrote:

What's the equivalent for -Djava.ext.dirs=null:../lib/spi in Java 11? I get this error when starting <app_s>:
[...]

In standard project template server.sh please replace:
spi="-Djava.locale.providers=SPI,JRE -Djava.ext.dirs=$(${prog} ${cpath} com.goldencode.util.PrintSystemProp java.ext.dirs):../lib/spi"
with
spi="-Djava.locale.providers=SPI,CLDR,COMPAT"

What is the cutoff point in Java for this change? I'd like to code up a check on the Java version, and make the decision in the server.sh script.

#82 Updated by Greg Shah almost 2 years ago

If I understand correctly, the old way only works for Java 8 and the new way works for Java 9 and above.

#83 Updated by Greg Shah almost 2 years ago

I'd like to code up a check on the Java version, and make the decision in the server.sh script.

This is a good idea.

#84 Updated by Hynek Cihlar almost 2 years ago

Code review 5567a revisions 14099..14102. The changes look good.

#85 Updated by Eugenie Lyzenko almost 2 years ago

The problems found in starting app_m with JDK11:

1. The FWD can not be compiled with Java 11 (to be fixed).
2. The libjvm.so location has been changed to /lib/server (fixable).
3. The server is unable to start (FWD compiled with JDK8):

...
[07/21/2022 17:02:30 GMT+03:00] (SecurityManager:SEVERE) {00000000:00000011:standard} Error getting AUTHTYPE java.io.EOFException
[07/21/2022 17:02:30 GMT+03:00] (SecurityManager:SEVERE) {00000000:00000012:standard} Error getting AUTHTYPE java.io.EOFException
Jul 21, 2022 5:02:30 PM com.goldencode.p2j.net.SessionManager createQueue
WARNING: SSL handshake failed, 2 attempts remaining
Jul 21, 2022 5:02:30 PM com.goldencode.p2j.net.SessionManager createQueue
WARNING: SSL handshake failed, 2 attempts remaining
[07/21/2022 17:02:35 GMT+03:00] (SecurityManager:SEVERE) {00000000:00000013:standard} Error getting AUTHTYPE java.io.EOFException
Jul 21, 2022 5:02:35 PM com.goldencode.p2j.net.SessionManager createQueue
WARNING: SSL handshake failed, 1 attempts remaining
[07/21/2022 17:02:36 GMT+03:00] (SecurityManager:SEVERE) {00000000:00000014:standard} Error getting AUTHTYPE java.io.EOFException
Jul 21, 2022 5:02:36 PM com.goldencode.p2j.net.SessionManager createQueue
WARNING: SSL handshake failed, 1 attempts remaining
[07/21/2022 17:02:41 GMT+03:00] (SecurityManager:SEVERE) {00000000:00000015:standard} Error getting AUTHTYPE java.io.EOFException
Problem retrieving the spawn command, using these settings: port [3333] host [localhost] alias [standard] UUID [72abe154-8f5b-4e41-91b9-af72527d525b]
javax.net.ssl.SSLHandshakeException: main:Handshake timed out.
    at com.goldencode.p2j.net.NIOSslSocket.<init>(NIOSslSocket.java:199)
    at com.goldencode.p2j.net.NetSocket$1.create(NetSocket.java:149)
    at com.goldencode.p2j.net.SessionManager.connect(SessionManager.java:1257)
    at com.goldencode.p2j.net.SessionManager.createQueue(SessionManager.java:1129)
    at com.goldencode.p2j.net.LeafSessionManager.connectDirect(LeafSessionManager.java:253)
    at com.goldencode.p2j.main.NativeSecureConnection.command(NativeSecureConnection.java:149)
[07/21/2022 17:02:41 GMT+03:00] (SecurityManager:SEVERE) {00000000:00000016:standard} Error getting AUTHTYPE java.io.EOFException
Problem retrieving the spawn command, using these settings: port [3333] host [localhost] alias [standard] UUID [1c4771d2-0152-4ae0-a186-bc6c86475a5c]
javax.net.ssl.SSLHandshakeException: main:Handshake timed out.
    at com.goldencode.p2j.net.NIOSslSocket.<init>(NIOSslSocket.java:199)
    at com.goldencode.p2j.net.NetSocket$1.create(NetSocket.java:149)
    at com.goldencode.p2j.net.SessionManager.connect(SessionManager.java:1257)
    at com.goldencode.p2j.net.SessionManager.createQueue(SessionManager.java:1129)
    at com.goldencode.p2j.net.LeafSessionManager.connectDirect(LeafSessionManager.java:253)
    at com.goldencode.p2j.main.NativeSecureConnection.command(NativeSecureConnection.java:149)
[07/21/2022 17:02:55 GMT+03:00] (ClientSpawner.spawn():SEVERE) {00000000:0000000C:standard} Failed spawn with exit code: 149
[07/21/2022 17:02:55 GMT+03:00] (ClientSpawner.spawn():SEVERE) {00000000:0000000F:standard} Failed spawn with exit code: 149
...

I have to get back to JDK 8 for couple of hours to test recent changed.

#86 Updated by Tomasz Domin almost 2 years ago

  • File 5567-1.fix added

Eugenie Lyzenko wrote:

The problems found in starting app_m with JDK11:

Thank you Eugenie.

1. The FWD can not be compiled with Java 11 (to be fixed).

Clean 5567a compiles with no issues. Do you have customizations in your FWD code ?

2. The libjvm.so location has been changed to /lib/server (fixable).

Fixed with 5567-1.diff

3. The server is unable to start (FWD compiled with JDK8):
[...]

Fixed with 5567-1.diff

I confirm this. I had to switch TLS Context from TLS to TLSv3, Greg - was I allowed to do this ?
hotel_gui works with no issues now.

Once confirmed 5567-1.diff works I'll commit it.

#87 Updated by Eugenie Lyzenko almost 2 years ago

Tomasz Domin wrote:

Eugenie Lyzenko wrote:

The problems found in starting app_m with JDK11:

Thank you Eugenie.

1. The FWD can not be compiled with Java 11 (to be fixed).

Clean 5567a compiles with no issues. Do you have customizations in your FWD code ?

I have tried 3821c. Should I use 5567a instead?

#88 Updated by Greg Shah almost 2 years ago

I had to switch TLS Context from TLS to TLSv3, Greg - was I allowed to do this ?

I don't have an objection, as long as it supports all the substitution of NIO, Bouncycastle and Conscrypt the same way that 3821c already supported it.

#89 Updated by Greg Shah almost 2 years ago

Yes, we are testing 5567a which is rebased from 3821c rev 14087.

#90 Updated by Eugenie Lyzenko almost 2 years ago

Greg Shah wrote:

Yes, we are testing 5567a which is rebased from 3821c rev 14087.

I'll update the environment and retest soon.

#91 Updated by Eugenie Lyzenko almost 2 years ago

Testing update:

1. Compilation is OK.
2. The Swing client works. I did some smoke tests with app_m and found no visible issues.
3. The Web client does not work, not application servers with known error:

...
[07/21/2022 20:51:42 GMT+03:00] (SecurityManager:SEVERE) {00000000:0000003D:standard} Error getting AUTHTYPE java.io.EOFException
Jul 21, 2022 8:51:42 PM com.goldencode.p2j.net.SessionManager createQueue
WARNING: SSL handshake failed, 2 attempts remaining
[07/21/2022 20:51:47 GMT+03:00] (SecurityManager:SEVERE) {00000000:0000003E:standard} Error getting AUTHTYPE java.io.EOFException
Jul 21, 2022 8:51:47 PM com.goldencode.p2j.net.SessionManager createQueue
WARNING: SSL handshake failed, 1 attempts remaining
[07/21/2022 20:51:52 GMT+03:00] (SecurityManager:SEVERE) {00000000:0000003F:standard} Error getting AUTHTYPE java.io.EOFException
Problem retrieving the spawn command, using these settings: port [3333] host [localhost] alias [standard] UUID [3ac5cc6d-422f-4507-a47e-98aab87af9c6]
javax.net.ssl.SSLHandshakeException: main:Handshake timed out.
    at com.goldencode.p2j.net.NIOSslSocket.<init>(NIOSslSocket.java:199)
    at com.goldencode.p2j.net.NetSocket$1.create(NetSocket.java:149)
    at com.goldencode.p2j.net.SessionManager.connect(SessionManager.java:1258)
    at com.goldencode.p2j.net.SessionManager.createQueue(SessionManager.java:1130)
    at com.goldencode.p2j.net.LeafSessionManager.connectDirect(LeafSessionManager.java:253)
    at com.goldencode.p2j.main.NativeSecureConnection.command(NativeSecureConnection.java:149)
[07/21/2022 20:52:06 GMT+03:00] (ClientSpawner.spawn():SEVERE) {00000000:0000000D:standard} Failed spawn with exit code: 149
...

So I think only NIO issue left here.

#92 Updated by Tomasz Domin almost 2 years ago

Eugenie Lyzenko wrote:

Testing update:

So I think only NIO issue left here.

Please Try 5567-1.diff

#93 Updated by Tomasz Domin almost 2 years ago

  • File deleted (5567-1.fix)

#94 Updated by Constantin Asofiei almost 2 years ago

Tomasz, are you able to run the FWD web client in Hotel GUI, using 5567a and Java 11?

#95 Updated by Tomasz Domin almost 2 years ago

Constantin Asofiei wrote:

Tomasz, are you able to run the FWD web client in Hotel GUI, using 5567a and Java 11?

Yes, in both - embedded and Virtual Desktop modes.

#96 Updated by Tomasz Domin almost 2 years ago

Constantin Asofiei wrote:

Tomasz, are you able to run the FWD web client in Hotel GUI, using 5567a and Java 11?

Note that I have separate folder spawner for every app/jvm combination.

#97 Updated by Eugenie Lyzenko almost 2 years ago

Tomasz Domin wrote:

Eugenie Lyzenko wrote:

Testing update:

So I think only NIO issue left here.

Please Try 5567-1.diff

OK. Testing.

BTW. I think this (NIO) is not JDK11 specific issue. I've tried to update my JDK8 install and got the similar issue. Probably just generic JDK "improvement".

#98 Updated by Eugenie Lyzenko almost 2 years ago

Eugenie Lyzenko wrote:

Tomasz Domin wrote:

Eugenie Lyzenko wrote:

Testing update:

So I think only NIO issue left here.

Please Try 5567-1.diff

OK. Testing.

BTW. I think this (NIO) is not JDK11 specific issue. I've tried to update my JDK8 install and got the similar issue. Probably just generic JDK "improvement".

No difference, same errors. I will try previous release of the JDK11.

#99 Updated by Tomasz Domin almost 2 years ago

Eugenie Lyzenko wrote:

No difference, same errors. I will try previous release of the JDK11.

My Java version

java -version
openjdk version "11.0.15" 2022-04-19
OpenJDK Runtime Environment (build 11.0.15+10-Ubuntu-0ubuntu0.20.04.1)
OpenJDK 64-Bit Server VM (build 11.0.15+10-Ubuntu-0ubuntu0.20.04.1, mixed mode, sharing)

#100 Updated by Eugenie Lyzenko almost 2 years ago

Tomasz Domin wrote:

Eugenie Lyzenko wrote:

No difference, same errors. I will try previous release of the JDK11.

My Java version
[...]

11_0_16 and 11_0_15 are the same for this point. Need to try even more step back. Checking *.14 ...

#101 Updated by Constantin Asofiei almost 2 years ago

Tomasz Domin wrote:

Constantin Asofiei wrote:

Tomasz, are you able to run the FWD web client in Hotel GUI, using 5567a and Java 11?

Yes, in both - embedded and Virtual Desktop modes.

OK, spawner for appserver uses a different mode. Try this command while in /opt/spawner folder:

ca@xever:/opt/spawner$ /opt/spawner/spawn 0 1234 localhost standard abc

and syslog will show this:
ca@xever:/opt/spawner$ tail -f /var/log/syslog | grep -i spawn
Jul 21 22:00:33 xever spawn[117843]: Error:-101 method:FindClass(NativeSecureConnection) (Success)

#102 Updated by Tomasz Domin almost 2 years ago

Constantin Asofiei wrote:

and syslog will show this:
[...]

Check ldd /opt/spawner/spawn
It should be:

tjd@tjd-workstation:~/projects/p2j_5567a$ ldd /opt/spawner_gui_5567a/spawn 
...
    *libjvm.so => /usr/lib/jvm/java-11-openjdk-amd64/lib/server/libjvm.so (0x00007f9017965000)*
...

Please also make sure that p2j.jar is the one with updated SSL configuration.

#103 Updated by Constantin Asofiei almost 2 years ago

Tomasz Domin wrote:

Constantin Asofiei wrote:

and syslog will show this:
[...]

Check ldd /opt/spawner/spawn
It should be:
[...]

Thanks, this was it, I forgot to re-link libp2j.so.

I get this when running some APIs in <app_s>, is not a show-stopper, but it needs to be fixed at some point I think:

WARNING: Illegal reflective access by com.esotericsoftware.reflectasm.AccessClassLoader (file:/path/to/app/deploy/lib/reflectasm-1.11.6.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.esotericsoftware.reflectasm.AccessClassLoader
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Otherwise, all the tests I have were ran OK, I didn't see any issues with Java 11.

#104 Updated by Eugenie Lyzenko almost 2 years ago

Eugenie Lyzenko wrote:

Tomasz Domin wrote:

Eugenie Lyzenko wrote:

No difference, same errors. I will try previous release of the JDK11.

My Java version
[...]

11_0_16 and 11_0_15 are the same for this point. Need to try even more step back. Checking *.14 ...

11_0_14 does not work too. We need another approach.

#105 Updated by Eugenie Lyzenko almost 2 years ago

Tomasz,

Seems like ctx = SSLContext.getInstance("TLSv1.2",... resolves the issue. Let me 10-15 min to test.

#106 Updated by Constantin Asofiei almost 2 years ago

My tests for <app_s> were done with this; I did not apply any other TLS patch, was done with 5567a as in rev 14102

ca@xever:~$ java -version
openjdk version "11.0.15" 2022-04-19
OpenJDK Runtime Environment (build 11.0.15+10-Ubuntu-0ubuntu0.20.04.1)
OpenJDK 64-Bit Server VM (build 11.0.15+10-Ubuntu-0ubuntu0.20.04.1, mixed mode, sharing)

This is using FWD's default SSL provider.

#107 Updated by Eugenie Lyzenko almost 2 years ago

Eugenie Lyzenko wrote:

Tomasz,

Seems like ctx = SSLContext.getInstance("TLSv1.2",... resolves the issue. Let me 10-15 min to test.

So far with the patch attached the app_m is working. I do not see any issues with smoke tests.

The JDK tested:

java version "11.0.16" 2022-07-19 LTS
Java(TM) SE Runtime Environment GraalVM EE 21.3.3 (build 11.0.16+11-LTS-jvmci-21.3-b20)
Java HotSpot(TM) 64-Bit Server VM GraalVM EE 21.3.3 (build 11.0.16+11-LTS-jvmci-21.3-b20, mixed mode, sharing)

#108 Updated by Tomasz Domin almost 2 years ago

Eugenie Lyzenko wrote:

Eugenie Lyzenko wrote:

Tomasz,

Seems like ctx = SSLContext.getInstance("TLSv1.2",... resolves the issue. Let me 10-15 min to test.

I've checked TLSv1.2 for hotel_gui and it also works correctly, so lets make TLSv1.2 the current version and check TLSv1.3 issues with your VM later.
I'll put it into repository.

#109 Updated by Greg Shah almost 2 years ago

Roger: After Tomasz has the latest changes checked in, please do a quick check of <app_c>.

Tomasz: What is the status of the todos in #5567-75? What about the performance concern in #5567-74?

#110 Updated by Roger Borrello almost 2 years ago

Greg Shah wrote:

Roger: After Tomasz has the latest changes checked in, please do a quick check of <app_c>.

The conversion I started this morning is still running using 5567a_14102. I assumed conversion using JDK 11 was part of the exercise. Is it OK just to update the runtime after conversion finishes?

#111 Updated by Greg Shah almost 2 years ago

I think so. Tomasz never mentioned this, but it wouldn't have been possible for any of our teammates to do testing otherwise.

#112 Updated by Greg Shah almost 2 years ago

Tomasz: It would be great if you post a list of the items to handle/consider when shifting a FWD installation from Java 8 to Java 11.

#113 Updated by Roger Borrello almost 2 years ago

Greg Shah wrote:

I'd like to code up a check on the Java version, and make the decision in the server.sh script.

This is a good idea.

For review is this snippet of code that can be used. In most server.sh, there is an option to only "test" the commands, hence the check so the 1.8 version doesn't attempt to execute the PrintSystemProp to generate the path at runtime. Also, the $prog is set to "java" and cut is a widespread utility. There were other ways with awk, but this is fairly terse. Comments welcome.

# spi is one way for Java 8, another for all other valid javas.
jver=$(eval java -version 2>&1 | grep version | cut -d\" -f2 | cut --complement -d'.' -f3);
if [ $jver = 1.8 ]; then
   echo "Using java 8" 
   if [ $test -eq 1 ] ; then
      spi="-Djava.locale.providers=SPI,JRE -Djava.ext.dirs=<TBD>:../lib/spi" 
   else
      spi="-Djava.locale.providers=SPI,JRE -Djava.ext.dirs=$(${prog} ${cpath} com.goldencode.util.PrintSystemProp java.ext.dirs):../lib/spi" 
   fi
elif [ $jver > 1.8 ]; then
   echo "Using java > 8" 
   spi="-Djava.locale.providers=SPI,CLDR,COMPAT" 
else
   echo "Using invalid java (8 or greater). Exiting." 
   exit 1
fi

#114 Updated by Tomasz Domin almost 2 years ago

Greg Shah wrote:

Questions for Tomasz:

  1. Is this branch ready for wider testing? If so, I'd like to make a push on testing for this branch to see if we can determine its readiness for merge into 3821c. If it is not ready for wider testing, please note the known issues.

After fixing issue with TLS I think its ready for wider testing.

  1. Have Hotel GUI and Hotel ChUI been checked recently?

Hotel GUI - yes,tested today. Hotel_ChUI - not recently

  1. Are there changes in this 5567a that result in any differences in converted output? If not, perhaps we can execute the same converted code with 3821c rev 14087 and 5567a rev 14102 and the results should be the same (unless there is a regression).

There are no changes to conversion process except a very small cleaning tasks for generated parser source code (see #5567#note-25). The cleaning only does sed 's/new Integer(/Integer.valueOf(' of generated parsers so generated code is functionally equal.

  • TJD: finish ChUI and CTRL-C regression tests comparison, retest Hotel GUI/Hotel ChUI if not recently tested

CTRL-C tests for 5567a are in progress on my laptop, I could quickly do Hotel ChUI afterwards.

Tomasz: What is the status of the todos in #5567-75?
adapt regression tests scripts for java 11 (90% completed)

Completed for regression test app.

Run CTRL-C with 5567a - should get 100% success as well

Running it now on my machine, going fine.

Run main-regression with 5567a - there would be errors, but baseline should be identical to 3821c.

I've tried main-regression for 3821c yesterday, but it still hangs/stops. Now I have harness logs, but didnt have time to analyse it today.
So main-regression for both 3821c and 5567a needs to be postponed once this issue (or other that appear) is fixed.

What about the performance concern in #5567-74?

I haven't started any performance analysis yet.

#115 Updated by Tomasz Domin almost 2 years ago

Tomasz Domin wrote:

Seems like ctx = SSLContext.getInstance("TLSv1.2",... resolves the issue. Let me 10-15 min to test.

I've checked TLSv1.2 for hotel_gui and it also works correctly, so lets make TLSv1.2 the current version and check TLSv1.3 issues with your VM later.
I'll put it into repository.

Revision 5567a/14103 is now with 5567-2.diff changes.

#116 Updated by Tomasz Domin almost 2 years ago

  • % Done changed from 80 to 90

CTRL-C tests for 5567a + 5739-4.diff + 5737a.diff completed successfully on my local workstation, I've uploaded results to ~tjd/testing/..../results/20220722_002850
(Note that 5739-4.diff + 5737a.diff are needed to make CTRL-C work correctly, but they are waiting for review)

I've also tested hotel_chui with 5567a/14103 and it also works just fine.

I have a question on updated apps - where to put all app specific temporary changes that are needed for 5567a ? Some of them are attached to this task.
For git repos I'd use branches, what about bzr repos ?

#117 Updated by Tomasz Domin almost 2 years ago

  • % Done changed from 90 to 80

Greg Shah wrote:

Tomasz: It would be great if you post a list of the items to handle/consider when shifting a FWD installation from Java 8 to Java 11.

Modify scripts (server.sh and build.xml/build_db.xml):
1. spi="-Djava.locale.providers=SPI,CLDR,COMPAT"
2. make sure fwdspi.jar is on classpath
3. make sure spawn is linked against Java 11 libjvm.so and clean/jar/reinstall spawner

#118 Updated by Tomasz Domin almost 2 years ago

  • % Done changed from 80 to 90

#119 Updated by Roger Borrello almost 2 years ago

I did a whole lot of smoke testing on <app_c>. Most of the issue were pilot error where I forgot to change from Java 8 (my default) to Java 11. There are some slight changes to the server.sh (as noted) and the client-terminal.sh, which I am trying to nail down so I can check these into the project.

  • Conversion was good
  • Database import was good
  • Server startup was good
  • ChUI client was good
  • Web client was good

I am looking forward to this being the default!

#120 Updated by Tomasz Domin over 1 year ago

Rebased to 3821c/14122, pushed 5567a/14138

Roger Borrello wrote:

Greg Shah wrote:

I'd like to code up a check on the Java version, and make the decision in the server.sh script.

This is a good idea.

I'll do the same for other applications instead of making java version related branches.

#121 Updated by Roger Borrello over 1 year ago

Tomasz Domin wrote:

I'll do the same for other applications instead of making java version related branches.

Welcome back, Tomasz! Please look at the latest server.sh in the <app_c> project, and review. There are some challenges when we are attempting to use runtime FWD that isn't linked to the project internals, and I try to accommodate. I'm open to improvements, if you see any possibilities.

The client.sh and client-terminal.sh also try to use the same logic.

#122 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

A customer did some testing on Java 11. They ran branch 6129a which is a modified version of an older 3821c. This 6129a branch was built in Java 8 and has no changes from 5567a. The customer was able to run on Java 11, but found that the performance was 10% to 20% slower than Java 8. This was on OpenJDK but he also tested th Amazon Coretto Java 8/11 with similar results.

He was worried about this issue as the a similar report: https://bugs.openjdk.org/browse/JDK-8252171

The bug is related with database performance degradation due to changes in I/O implementation.
So we need to check database performance under Java 8 vs under Java 11.

Were there any comparison benchmarks done in the past ?
Like comparing P2J operations vs native ? Or P2J on hibernate ORM (I guess this is old way) vs P2J with native ORM ?

If not so a kind of database benchmark app should be considered, I've found ATM Benchmark Tool, but not sure if I can use it yet.

Another option would be running a set of harness scenarios constructed in a specific way - doing a lot of repetitive operations. This would be closer to real-world application performance testings.

I will check both options.

#123 Updated by Greg Shah over 1 year ago

We have indeed done a large amount of testing of the persistence layer/database performance in FWD. There are many areas were working on.

We can ignore Hibernate. We have not used it in a long time. It was substantially slower than our current approach and it should not matter in the Java 8 vs Java 11 comparison.

Using the Harness is a good idea. The processing is fairing database intensive and represents real world customer scenarios. I would start with just looking at performance differences between Java 8/11 runs of the Harness.

Eric: What other database performance tests can be easily try?

#124 Updated by Eugenie Lyzenko over 1 year ago

Two notes here:

1. The patch in 5567-2.diff is need to be applied even for further versions of the Java 8, not only when shifting to Java 11.
2. The performance decrease from shifting to Java 11 is an expected feature. This is not a bug but cost for using more recent JDK.

#125 Updated by Greg Shah over 1 year ago

The performance decrease from shifting to Java 11 is an expected feature. This is not a bug but cost for using more recent JDK.

Let's quantify this by testing real application code. I would not have expected it.

#126 Updated by Tomasz Domin over 1 year ago

I've made changes to build scripts to make this branch compatible with Java 8 again
So now JVM used during compiling determines required JVM runtime:
- java 8 build requires java 8+ for runtime
- java 11 build requires java 11+ for runtime

I haven't tested other JVM versions.
Committed revision 14139.

#127 Updated by Roger Borrello over 1 year ago

Tomasz Domin wrote:

I've made changes to build scripts to make this branch compatible with Java 8 again
So now JVM used during compiling determines required JVM runtime:
- java 8 build requires java 8+ for runtime
- java 11 build requires java 11+ for runtime

I haven't tested other JVM versions.
Committed revision 14139.

One change you have made to build.gradle results in success in my attempts to utilize the dist/convert results to perform conversions of applications. Previously I was receiving:

     [java] Importing 'standard.df' for schema 'standard'...
     [java] Exception in thread "main" java.lang.NoClassDefFoundError: org/w3c/dom/ElementTraversal
     [java]     at java.lang.ClassLoader.defineClass1(Native Method)
     [java]     at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
     [java]     at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
     [java]     at java.net.URLClassLoader.defineClass(URLClassLoader.java:473)
     [java]     at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
     [java]     at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
     [java]     at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
     [java]     at java.security.AccessController.doPrivileged(Native Method)
     [java]     at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
     [java]     at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
     [java]     at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
     [java]     at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
     [java]     at org.apache.xerces.parsers.AbstractDOMParser.startDocument(Unknown Source)
     [java]     at org.apache.xerces.impl.dtd.XMLDTDValidator.startDocument(Unknown Source)
     [java]     at org.apache.xerces.impl.XMLDocumentScannerImpl.startEntity(Unknown Source)
     [java]     at org.apache.xerces.impl.XMLVersionDetector.startDocumentParsing(Unknown Source)
     [java]     at org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
     [java]     at org.apache.xerces.parsers.XML11Configuration.parse(Unknown Source)
     [java]     at org.apache.xerces.parsers.XMLParser.parse(Unknown Source)
     [java] 
     [java]     at org.apache.xerces.parsers.DOMParser.parse(Unknown Source)------------------------------------------------------------------------------
     [java]     at org.apache.xerces.jaxp.DocumentBuilderImpl.parse(Unknown Source)

The dependencies change that removed the forcing of xml-apis to 1.3.04 was the removal of this:

    fwdCCS (group: 'xml-apis', name: 'xml-apis', version: '1.3.04') {
       force = true
    }

and the inclusion of a new conditional
configurations.all {
// remove apis provided by java 9+
    if (JavaVersion.current().isJava9Compatible())
    {
       exclude group: 'xml-apis', module: "xml-apis" 
       exclude group: 'org.slf4j', module: 'slf4j-simple'
       exclude group: 'org.apache.xmlgraphics', module: 'batik-ext'
       exclude group: 'stax', module: 'stax-api'
    }
}

This results in the usage of xml-apis-1.4.01.jar, which seems to alleviate a conversion error above. Is the newer version of xml-apis compatible with Java 8?

#128 Updated by Tomasz Domin over 1 year ago

I've made it to run main_regression tests on my local workstation. It took some time as each run takes about 6-7 hours (build + database import + tests).

I've found two issues that made the tests hang:

  • SSH configuration preventing spawning needed session count on devsrv01. Greg/Eric - I will send you a separate email on this.
  • bug in harness which results in hanging TestPlan (PerThreadElement waits for all defined threads to finish despite some of them may have failed during initialization). I've extended logging to report this case (exception on ssh connect) but have not fixed the issue itself

There are some of tests failing due different screen view in the current version, I will isolate and report these issues.

I will run the tests for 3821c once changes on devsvr01 are done.

#129 Updated by Tomasz Domin over 1 year ago

Roger Borrello wrote:

This results in the usage of xml-apis-1.4.01.jar, which seems to alleviate a conversion error above. Is the newer version of xml-apis compatible with Java 8?

Yes, it may be a side effect :) of removing 'xml-apis' '1.3.04' forced version, as XML apis are provided by JDK 11+ so I have removed external imports that were needed by Java8.
Dont know why version '1.3.04' was forced, but for Java 11 it has to be removed anyway (unless we play with modules), as its provided by JDK itself.

#130 Updated by Tomasz Domin over 1 year ago

weekly rebase to 3821c/14131, committed 5821c/14149

#131 Updated by Greg Shah over 1 year ago

It took some time as each run takes about 6-7 hours (build + database import + tests).

From my memory about expected timings:

  • Build would be expected to take about an 60-70 minutes.
  • Database restore should take 30-40 minutes.
  • Testing should take 3 hours and 40 minutes if there are not any unexpected failures.

If the testing is what is taking the extra time, then it is likely that there are a bunch of failures that are causing the delays.

I've made changes to build scripts to make this branch compatible with Java 8 again
So now JVM used during compiling determines required JVM runtime:

I like this a lot. It gives us some options for testing and "falling back" which customers may need.

#132 Updated by Roger Borrello over 1 year ago

Tomasz, I just checked build.gradle into 3821c_14140 with the update mentioned above. Is there any way both 3821c build.gradle and 5567 build.gradle could be the same file, using JavaVersion.current().isJava11Compatible where needed? There are just a small number of differences between them.

I do want to point out that com.esotericsoftware is in both the fwdClientServer (updated from 1.11.3 to 1.11.6) and in the fwdServer group (still at 1.11.3). I don't think it's necessary to be in fwdServer, do you agree?

#133 Updated by Tomasz Domin over 1 year ago

Roger Borrello wrote:

Tomasz, I just checked build.gradle into 3821c_14140 with the update mentioned above. Is there any way both 3821c build.gradle and 5567 build.gradle could be the same file, using JavaVersion.current().isJava11Compatible where needed? There are just a small number of differences between them.

I guess the goal is to merge 5567a into 3821c, so they'd become single file. Currently 5567a is a bit behind, as I need to rebase time to time (once a week).

I do want to point out that com.esotericsoftware is in both the fwdClientServer (updated from 1.11.3 to 1.11.6) and in the fwdServer group (still at 1.11.3). I don't think it's necessary to be in fwdServer, do you agree?

Yes, as fwdServer extends from fwdClientServer.

#134 Updated by Greg Shah over 1 year ago

If 5567a has the same ChUI regression testing results as 3821c, then the only thing holding us back from merging 5567a into 3821c is the concern about performance. On the other hand, if we can continue to use Java 8 or Java 11 as needed for 3821c, then perhaps even the performance concern will not hold us back.

  • What is the process to use Java 8 vs Java 11 in 5567a?
  • What performance comparisons have been done? Do you have any comparisons using ChUI regression testing (for example)?

#135 Updated by Greg Shah over 1 year ago

Tomasz: Do you have comparable ChUI regression test results between 3821c and 5567a?

#136 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

Tomasz: Do you have comparable ChUI regression test results between 3821c and 5567a?

Yes I have, they are very, very similar, but not the same.
Please check #6667#note-41 for details.

#137 Updated by Greg Shah over 1 year ago

That is probably within "normal range" of results. The ChUI test set is not 100% reliable. There are false negatives (a test that seems to fail but it really is passing) that pass most of the time and sometimes fail. These cause the results to be hard to interpret, especially for a single run. Some of the tests may have hidden dependencies which are broken based on timing only. For example, a test may depend upon the database state that is changed by another test, most of the time that other test completes first and the dependent test works but sometimes the order is different and the dependent test fails (e.g. different data in the output on a screen or in a report). If the dependency was not made explicit in the test suite, then this will contribute to our false negatives.

We are so busy that we have not gone back and resolved these false negatives. I think there are 10-15 tests that are probably in this category. If you run the test suite multiple times and there is no single unexpected test that fails in every run, then we accept that as passing. I think you are not seeing any real difference between Java 8 and Java 11.

#138 Updated by Greg Shah over 1 year ago

Let me know when you have some results on the performance comparison. I think that is the remaining blocker. Even if it is a real problem, if we can continue to run Java 8 without seeing the performance issue, then this "workaround" means that we can still merge 5567a into 3821c.

I'm hopeful that we can do this in the next couple of days.

#139 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

Let me know when you have some results on the performance comparison. I think that is the remaining blocker. Even if it is a real problem, if we can continue to run Java 8 without seeing the performance issue, then this "workaround" means that we can still merge 5567a into 3821c.

I'm hopeful that we can do this in the next couple of days.

I will focus on performance test this week.

#140 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

Let me know when you have some results on the performance comparison. I think that is the remaining blocker. Even if it is a real problem, if we can continue to run Java 8 without seeing the performance issue, then this "workaround" means that we can still merge 5567a into 3821c.

I'm hopeful that we can do this in the next couple of days.

I've completed initial round of performance testing.
Depending on TestSet nature (depending on area of the platform tested) Java 11 seems to be slower by 5-30% compared to Java 8.
It seems the more complex scenario the difference is bigger.
I will do more tests to get better results.

#141 Updated by Tomasz Domin over 1 year ago

Rebased to 3821c/14173, pushed 5567a/14191 for performance comparison tests.

#143 Updated by Greg Shah over 1 year ago

OK, so far this looks like it is consistent with the customer's early findings (i.e. Java 8 is 10-20% faster than Java 11). How much more testing do you need to be confident in your numbers?

If we merge this to 3821c, what must be done to remain on Java 8? We'll need documentation in regard to this, since all of our customers use 3821c or are moving to it.

Finally, what is the status of 5567a? I understand you are using the 5739 changes to make the ChUI tests work better. I'm not clear on the other patches from #6690-2. Does 5567a need changes before it is safe or are these patches only because of regressions that affect the ChUI tests?

#144 Updated by Greg Shah over 1 year ago

#145 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

OK, so far this looks like it is consistent with the customer's early findings (i.e. Java 8 is 10-20% faster than Java 11). How much more testing do you need to be confident in your numbers?

I think it will be done by the end this week - I need to start testing loop overnight and I need 2-3 nights once having a stable baseline (testSkip file)

If we merge this to 3821c, what must be done to remain on Java 8? We'll need documentation in regard to this, since all of our customers use 3821c or are moving to it.

After merging 5567a into 3821c nothing needs to be done to remain on Java 8 - merged p2j shall stay buildable and runnable with Java 8 probably with no major issues, except there may be some side effects as a result of upgrading some of dependencies (probably like the one Roger has). To make sure I can run regression tests also with java8 built version of 5567a (if I have time for this).

Finally, what is the status of 5567a? I understand you are using the 5739 changes to make the ChUI tests work better. I'm not clear on the other patches from #6690-2. Does 5567a need changes before it is safe or are these patches only because of regressions that affect the ChUI tests?

5567a is build on top of the recent 3821c and as I rebase it once-twice a week it is in-line with latest 3821c changes.

Patches 5739-4.diff and 5737a.diff are only needed for proper ChUI regression testing and since they are not reviewed/accepted to be merged into 3821c so I am adding them manually before testing to improve testing results/success rate (assuming they will be accepted). The other patches are already in 3821c, so they will be merged into 5567a on next rebase.

#146 Updated by Tomasz Domin over 1 year ago

I am done with performance comparison between Java 8 and Java 11 in real-life like test scenarios.
I've managed to get valid results (IMO) by running performance tests.
Depending on test nature Java 11 is slower compared to Java 8 by 5-45%.
It seems the more complex the scenario/operation is in terms of I/O operations the bigger the difference is.
I'd like to perform one more performance test - I'd like to check how much CPU speed affects the results.
I will reduce CPU clock by 50% (as I cannot make it higher) and reexecute Java 11 tests, so I would know (more or less) if doubling CPU speed would compensate slower JVM performance.

#147 Updated by Tomasz Domin over 1 year ago

Rebased to 3821c/14205, pushed 5567a/14223

#148 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

OK, so far this looks like it is consistent with the customer's early findings (i.e. Java 8 is 10-20% faster than Java 11). How much more testing do you need to be confident in your numbers?

Greg, do we have more information ? Like a bit more details on what it slower, maybe some profiling information (except the mentioned bug report) ?
Maybe there is a workaround to the issue, as in general (and from my experience), Java 11 should be faster or at least only a bit slower.

#149 Updated by Greg Shah over 1 year ago

Tomasz Domin wrote:

Greg Shah wrote:

OK, so far this looks like it is consistent with the customer's early findings (i.e. Java 8 is 10-20% faster than Java 11). How much more testing do you need to be confident in your numbers?

Greg, do we have more information ? Like a bit more details on what it slower, maybe some profiling information (except the mentioned bug report) ?

We have the customer's testcase, but no additional information.

Maybe there is a workaround to the issue, as in general (and from my experience), Java 11 should be faster or at least only a bit slower.

The question here is whether it makes sense to work on this or if it makes more sense to look at Java 17 (#6692). Ultimately, Java 17 is where we want to be. If it is at or better than Java 8 performance, then we would move to 17 and not waste time on 11.

#150 Updated by Greg Shah over 1 year ago

To be more clear, the customer has not done any further testing or analysis and they are not planning to do so. Your testing has been much more extensive and seems to corroborate their early results. Do you think we need more analysis or can we conclude that Java 11 does seem to have a real performance impact compared to Java 8 for our "normal usage"?

If so, I'm OK with merging 5567a into 3821c so long as we document:

  • what is needed to use Java 8 vs Java 11
  • any steps needed to accept the changes without any change in JDK
  • the recommendation to stick with Java 8 for performance reasons (and why)

Otherwise, if we need more analysis, please suggest the next steps.

#151 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

Otherwise, if we need more analysis, please suggest the next steps.

I have another idea - taking this opportunity of upgrading to newer JVM to upgrade all external dependencies (like external jar files).
I only upgraded external dependencies when it was necessary to make given library to work under Java 11.
But I think we should do an upgrade of all libraries to current/latest releases.

#152 Updated by Greg Shah over 1 year ago

But I think we should do an upgrade of all libraries to current/latest releases.

I agree with the idea that we want to have all dependencies at a current level. However, I hesitate to do this for this current task, since it adds extra work and extra testing.

Why not defer that until #6692, where more dependencies probably need to be updated to match Java 17?

#153 Updated by Tomasz Domin over 1 year ago

Why not defer that until #6692, where more dependencies probably need to be updated to match Java 17?

My idea was to encourage customers to migrate to Java 11 (and then Java 17) runtime by providing a runtime composed of components in the latest version, despite the environment being a bit slower (Jav 17 to be checked).

#154 Updated by Greg Shah over 1 year ago

My idea was to encourage customers to migrate to Java 11 (and then Java 17) runtime by providing a runtime composed of components in the latest version, despite the environment being a bit slower (Jav 17 to be checked).

No one can accept a 20% performance degradation to use Java 11. But we need these changes to take the next step. I'm OK with us starting work on #6692 an doing a first pass which updates all dependencies to the latest level (without moving to Java 17).

Tomasz: I'd really like to merge 5567a into 3821c without any additional development that is not strictly needed. What is needed to get there?

#155 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

Tomasz: I'd really like to merge 5567a into 3821c without any additional development that is not strictly needed. What is needed to get there?

Ok, I will try to close this task ASAP.

As 5567a was outdated I've updated 5567a to the latest 3821c/14317
In order to close this task (with minimal risk) I need to complete testing of both - 3821c and 5567a for the same 3821c revision/code base
As last tests results are outdated I will rerun them again on the GCD test environment: both regression tests and ctrl-c tests for 3821c/14316 and its equivalent 5567a/14335.
If results are the same I think we could safely merge 5567a branch into 3821c.
I will do it this week.

#156 Updated by Tomasz Domin over 1 year ago

I've executed main-regression and ctrl-c tests for following configurations with full conversion and build done prior to testing:
- java 8 on 3821c rev 14317
- java 8 on 5567a rev 14335
- java 11 on 5567a rev 14335

Java 8 version: openjdk version "1.8.0_282"
Java 11 version: java version "11.0.15" 2022-04-19 LTS

Summary:
  • ctrl-c test results are 100% identical
  • main-regression test between 3821c and 5567a are identical to the extent possible due to server load (5567a results are better) and identical between java 8 on 5567a and java 11 on 5567a.

I think it is safe to merge 5567a into 3821c.

There is one small thing - there are two warnings when compiling for Java 11 which are result of recent changes, they are not critical I think they can be corrected after the merge.

I think a new rebase to current 3821c is needed anyway as 5567a is now based to rev 14317 and a final review of 5567a would be helpful.

#157 Updated by Greg Shah over 1 year ago

OK, please go ahead with the final rebase.

Hynek: Please do a final code review on that rebased version.

If all looks good, Tomasz can go ahead and merge.

#158 Updated by Tomasz Domin over 1 year ago

Rebased to 3821@14327, pushed 5567a@14345
Waiting for the review.

#159 Updated by Hynek Cihlar over 1 year ago

Tomasz Domin wrote:

Rebased to 3821@14327, pushed 5567a@14345
Waiting for the review.

This is in progress. Most likely I will not be able to finish it by EOD.

#160 Updated by Hynek Cihlar over 1 year ago

Tomasz, you downgraded Keikai version to 5.8.1. Why was this needed? I tried to build the project with 5.9.0 and Java 11 and it succeeded.

#161 Updated by Tomasz Domin over 1 year ago

Hynek Cihlar wrote:

Tomasz, you downgraded Keikai version to 5.8.1. Why was this needed? I tried to build the project with 5.9.0 and Java 11 and it succeeded.

You probably have not-the-latest Java 11 was that was accepting path like "../../.." in jar files. The new ones does not allow for it AFAIK due to security reasons.
https://bugs.openjdk.org/browse/JDK-8283486

I have openjdk version "11.0.16" 2022-07-19

For me I get:

[ant:exec]   ZipException opening "keikai-pdf-5.9.0.jar": ZIP file can't be opened as a file system because entry "/../../../LICENSE.TXT" has a '.' or '..' element in its name
[ant:exec] /home/tjd/projects/p2j_5567a/ext/sheet/src/main/java/com/goldencode/p2j/ext/sheet/formula/Weeknum.java:77: error: cannot access com.goldencode.p2j.ext.sheet.formula
[ant:exec] package com.goldencode.p2j.ext.sheet.formula;
[ant:exec] ^
[ant:exec]   ZipException opening "keikai-pdf-5.9.0.jar": ZIP file can't be opened as a file system because entry "/../../../LICENSE.TXT" has a '.' or '..' element in its name
[ant:exec] /home/tjd/projects/p2j_5567a/ext/sheet/src/main/java/com/goldencode/p2j/ext/sheet/web/SheetControllerImpl.java:140: error: cannot access com.goldencode.p2j.ext.sheet
[ant:exec] public class SheetControllerImpl

What about upgrading to keikai 5.10.0 ?

#162 Updated by Hynek Cihlar over 1 year ago

Tomasz Domin wrote:

For me I get:
[...]

What about upgrading to keikai 5.10.0 ?

The file LICENSE.TXT was added by us, so the fix will be to zip it to the correct location.

#163 Updated by Hynek Cihlar over 1 year ago

Code review 5567a.

AnnotatedAst, PaintStructure, GuiWebSocket, FileStream only contain changes in the file header.

I checked in obvious issues I found in 5567a revision 14346. Tomasz, please review them.

Btw. build time on my computer went down from 02:08 to 01:10 with Java 11.

#164 Updated by Roger Borrello over 1 year ago

Is there a strategy to approach the switch to Java 11? For example, can we leave our project build the same, but utilize Java 11-built FWD as a first phase, then switch our projects to build with Java-11 as a second phase? Or should both be switched at once?

#165 Updated by Tomasz Domin over 1 year ago

Roger Borrello wrote:

Is there a strategy to approach the switch to Java 11? For example, can we leave our project build the same, but utilize Java 11-built FWD as a first phase, then switch our projects to build with Java-11 as a second phase? Or should both be switched at once?

There are two areas:
Build scripts shall detect Java version runtime and configure environment according to the detected java version.
P2J Runtime scripts shall do the same - today I've updated scripts for hotel and hotel_gui, I will update APP_M runtime script, regarding other apps its probably case by case. (btw your changes Roger to hotel runtime scripts work perfectly well).

It is possible to:
- build with java 8 and run with java 8 (its obvious)
- build with java 11 and run with java 11 (tested)
It should be possible to build with java 8 and run with java 11, but it was tested yet

As I understood for now Java 8 stays as main runtime platform.
If we plan to support Java 11 I'd suggest to add Java 11 environment to CI/CD once it works in parallel to Java 8 version, before that or before any of customers would switch once a month (or once a week) someone needs to build Java 11 version and test.

#166 Updated by Tomasz Domin over 1 year ago

Hynek Cihlar wrote:

Tomasz Domin wrote:

For me I get:
[...]

What about upgrading to keikai 5.10.0 ?

The file LICENSE.TXT was added by us, so the fix will be to zip it to the correct location.

AFAIK I dont have write access to GCD maven repository.
Greg - can you or someone having the access repack all keikai 5.9.0 jars to move ../../../LICENSE.TXT to LICENSE.TXT
Or let me know and I will do it and will send you changed files.
Then we can switch back to keikai 5.9.0.

#167 Updated by Hynek Cihlar over 1 year ago

Tomasz Domin wrote:

Hynek Cihlar wrote:

Tomasz Domin wrote:

For me I get:
[...]

What about upgrading to keikai 5.10.0 ?

The file LICENSE.TXT was added by us, so the fix will be to zip it to the correct location.

AFAIK I dont have write access to GCD maven repository.
Greg - can you or someone having the access repack all keikai 5.9.0 jars to move ../../../LICENSE.TXT to LICENSE.TXT
Or let me know and I will do it and will send you changed files.
Then we can switch back to keikai 5.9.0.

Guys, let's not duplicate work. Tomasz, just deliver the 5.8.1 in 3821c and I will update to 5.10 with correct LICENSE.TXT.

#168 Updated by Tomasz Domin over 1 year ago

I've reviewed the previous commit, removed doubled comments, removed header only changes. The reference to Keikai 5.8.1 stayed unchanged according to Hynek's suggestion.
Rebased to 3821c/14332, pushed 5567a/14352

#169 Updated by Hynek Cihlar over 1 year ago

Greg, I copied Keikai 5.10.0 dependencies with the LICENSE.TXT files added in devsrv01:/tmp/keikai-5.10.0.zip. Please let me know when you upload it in the artifact repository and I will commit the necessary FWD changes to move to 5.10.0.

#170 Updated by Greg Shah over 1 year ago

Hynek Cihlar wrote:

Greg, I copied Keikai 5.10.0 dependencies with the LICENSE.TXT files added in devsrv01:/tmp/keikai-5.10.0.zip. Please let me know when you upload it in the artifact repository and I will commit the necessary FWD changes to move to 5.10.0.

I applied all changes to the artifacts/ directory. Did you expect to overwrite the many maven-metadata-local.xml that already existed there? Does that break the older builds?

#171 Updated by Hynek Cihlar over 1 year ago

Greg Shah wrote:

Hynek Cihlar wrote:

Greg, I copied Keikai 5.10.0 dependencies with the LICENSE.TXT files added in devsrv01:/tmp/keikai-5.10.0.zip. Please let me know when you upload it in the artifact repository and I will commit the necessary FWD changes to move to 5.10.0.

I applied all changes to the artifacts/ directory. Did you expect to overwrite the many maven-metadata-local.xml that already existed there? Does that break the older builds?

maven-metadata-local.xml isn't used for the artifact resolution during build. It contains metadata for maven when it fetches the files from remote repositories. I suggest you leave them there in the GCD repo.

#172 Updated by Hynek Cihlar over 1 year ago

Keikai upgraded in 3821c revision 14338. Please review.

#173 Updated by Greg Shah over 1 year ago

Code Review Task Branch 3821c Revision 14338

No objections.

#174 Updated by Tomasz Domin over 1 year ago

Rebased to 3821c/14342, pushed 5567a/14362

#175 Updated by Greg Shah over 1 year ago

Is there any reason that we should not merge this into 3821c?

#176 Updated by Tomasz Domin over 1 year ago

Greg Shah wrote:

Is there any reason that we should not merge this into 3821c?

I dont know any - its clear to merge it into 3821c.
Note there is a new commit in 3821c so another rebase is needed (as I understand its the requirement for the merge).

#177 Updated by Greg Shah over 1 year ago

Right. Go ahead and rebase and then merge into 3821c.

Is there anything that the team or our customers need to be aware of when taking this new revision? For example, are all customer projects already updated with the SPI changes? Are there any issues if a system is misconfigured such that there are different JDKs used for javac and java execution? How does one shift between version?

I want to make sure that this is seamless. When the merge happens, we need to send a notification to the team with this list of advice.

#178 Updated by Tomasz Domin over 1 year ago

I've prepared a wiki page on Java 11 support and migration, for now its internal and probably needs to be discussed and updated:

Java_11_Support_and_Migration (GESLE: content moved to Switching Between Java 8 and Java 11)

I will rebase and try to merge the branch today.

#179 Updated by Tomasz Domin over 1 year ago

Merged into 3821c rev 14344.

#180 Updated by Tomasz Domin over 1 year ago

  • Status changed from WIP to Review
  • % Done changed from 90 to 100

#181 Updated by Greg Shah over 1 year ago

  • % Done changed from 100 to 90

I've made changes to Software Dependencies and have added Switching Between Java 8 and Java 11 to explain this support as an external/public document.

Tomasz: Please review the changes in those documents and let me know if anything needs to be modified.

#182 Updated by Greg Shah over 1 year ago

  • % Done changed from 90 to 100

#183 Updated by Tomasz Domin over 1 year ago

  • Status changed from Review to Feedback

Page now looks much better and its easier to read and understand it, thank you.

  • "(Java 11 policy/preference to be confirmed)." - this is to be removed, I didnt know the GCD official policy regarding Java 11 recommendations to customers, now its clear.
  • "Unsupported Configurations" - Actually I was thinking about bringing these configuration under support, I simply didnt have time to test it, shall I work on it further ? Not sure if this would have any benefits to customers.
  • I would add a note that Java 9 and Java 10 are not tested/not supported to make it explicit. I presume not tested is not supported even if it may work for some configurations.
  • Internal document has been migrated to public wiki, are we going to keep it ?

#184 Updated by Greg Shah over 1 year ago

"(Java 11 policy/preference to be confirmed)." - this is to be removed

Done

"Unsupported Configurations" - Actually I was thinking about bringing these configuration under support, I simply didnt have time to test it, shall I work on it further ?

No, I don't see much value there. We want to focus on Java 17, I think.

I would add a note that Java 9 and Java 10 are not tested/not supported to make it explicit.

Good point. Done.

I presume not tested is not supported even if it may work for some configurations.

Correct

Internal document has been migrated to public wiki, are we going to keep it ?

I'll delete it.

#185 Updated by Greg Shah over 1 year ago

  • Status changed from Feedback to Closed

#186 Updated by Roger Borrello over 1 year ago

I checked out hotel_gui, ran the prepare for linux (and H2), and I get this when I try to import:

create.db.h2:
     [java] org.h2.jdbc.JdbcSQLDataException: Invalid value "en_US_iso88591_fwd_basic" for parameter "collation"; SQL statement:
     [java] set collation "en_US_iso88591_fwd_basic" [90008-200]
     [java]     at org.h2.message.DbException.getJdbcSQLException(DbException.java:590)
     [java]     at org.h2.message.DbException.getJdbcSQLException(DbException.java:429)
     [java]     at org.h2.message.DbException.get(DbException.java:205)
     [java]     at org.h2.message.DbException.getInvalidValueException(DbException.java:280)
     [java]     at org.h2.command.Parser.parseSetCollation(Parser.java:7476)
     [java]     at org.h2.command.Parser.parseSet(Parser.java:7246)
     [java]     at org.h2.command.Parser.parsePrepared(Parser.java:987)
     [java]     at org.h2.command.Parser.parse(Parser.java:843)
     [java]     at org.h2.command.Parser.parse(Parser.java:815)
     [java]     at org.h2.command.Parser.prepareCommand(Parser.java:738)
     [java]     at org.h2.engine.Session.prepareLocal(Session.java:657)
     [java]     at org.h2.engine.Session.prepareCommand(Session.java:595)
     [java]     at org.h2.jdbc.JdbcConnection.prepareCommand(JdbcConnection.java:1235)
     [java]     at org.h2.jdbc.JdbcStatement.executeInternal(JdbcStatement.java:212)
     [java]     at org.h2.jdbc.JdbcStatement.execute(JdbcStatement.java:201)
     [java]     at org.h2.tools.RunScript.process(RunScript.java:261)
     [java]     at org.h2.tools.RunScript.process(RunScript.java:192)
     [java]     at org.h2.tools.RunScript.process(RunScript.java:328)
     [java]     at org.h2.tools.RunScript.runTool(RunScript.java:143)
     [java]     at org.h2.tools.RunScript.main(RunScript.java:70)

Is this related to Java 11 changes to build_db.xml or something else?

#187 Updated by Roger Borrello over 1 year ago

Roger Borrello wrote:

I checked out hotel_gui, ran the prepare for linux (and H2), and I get this when I try to import:
[...]

Is this related to Java 11 changes to build_db.xml or something else?

The property java.ext.dirs=deploy/lib/spi needs to be set. Tomasz, was there something missing from your hotel_gui check-in? Should this be in build.properties?

#188 Updated by Tomasz Domin over 1 year ago

Roger Borrello wrote:

Roger Borrello wrote:

I checked out hotel_gui, ran the prepare for linux (and H2), and I get this when I try to import:
[...]

Is this related to Java 11 changes to build_db.xml or something else?

The property java.ext.dirs=deploy/lib/spi needs to be set. Tomasz, was there something missing from your hotel_gui check-in? Should this be in build.properties?

Correct, I am working on this now.

#189 Updated by Tomasz Domin over 1 year ago

Roger Borrello wrote:

Roger Borrello wrote:

I checked out hotel_gui, ran the prepare for linux (and H2), and I get this when I try to import:
[...]

Is this related to Java 11 changes to build_db.xml or something else?

The property java.ext.dirs=deploy/lib/spi needs to be set. Tomasz, was there something missing from your hotel_gui check-in? Should this be in build.properties?

It seems I skipped commiting changes to build.xml
Please update hotel_gui app from its repository.
Thank you for your report.

Also available in: Atom PDF