Project

General

Profile

Bug #7793

incremental conversion with deleted table from .df

Added by Constantin Asofiei almost 3 years ago. Updated 9 months ago.

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

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
trunk/16220
production:
No
env_name:
topics:

Related issues

Related to Conversion Tools - Feature #6083: schema (.df) changes in incremental conversion mode should only reconvert those programs that reference the changed schema Closed
Related to Conversion Tools - Bug #10639: Incremental conversion - unexpected DMO generation for space-only changes Review

History

#1 Updated by Constantin Asofiei almost 3 years ago

If an incremental conversion is done with a .df file which removes a table, the DMO remains in place, both as .java and .class, if there is an actual build.

The actual Java build will get cleaned up only after a full-conversion, as currently incremental conversion is not smart enough to remove the DMO .java file. Or if the DMO .java file is removed manually and the DMO jar rebuilt.

We need a way to validate the DMOs to exist in the 'meta schema' for that database, and log a warning in the FWD server log - the protection needs to be in DatabaseManager.getDatabaseDMOs which uses reflection to scan the application's DMO package for DataModelObject types.

Otherwise, the error is pretty confusing: meta schema load can fail with i.e. dump-name index collision, and the FWD server log will end up with errors like these:

Caused by: com.goldencode.p2j.persist.PersistenceException: Error running script against embedded database
Caused by: org.h2.jdbc.JdbcSQLSyntaxErrorException: Table "META_AREA" already exists; SQL statement:create table meta_area (
   recid bigint not null,
   area_version integer,
   area_number integer not null,
   area_name varchar_ignorecase not null,
   area_type integer not null,
   area_block integer,
   area_attrib integer not null,
   area_clustersize integer,
   area_blocksize integer not null,
   area_recbits integer,
   area_extents integer,
   area_misc1 integer,
   area_misc2 integer,
   area_misc3 integer,
   area_misc4 integer,
   area_misc5 integer,
   area_misc6 integer,
   area_misc7 integer,
   area_misc8 integer,
   primary key (recid)

because the initial abend will be via activator.activate(); in DatabaseManager.activateAndRegister:
   private static void activateAndRegister(Integer sessionId, Database db, Activator activator)
   throws PersistenceException
   {
      AutoConnected connected = dbSessions.computeIfAbsent(db, AutoConnected::new);
      connected.writeLock().lock();
      try
      {
         if (!connected.isActivated())
         {
            activator.activate();
         }
         connected.add(sessionId);
      }
      finally
      {
         connected.writeLock().unlock();
      }
      context.get().autoConnected.add(db);
   }

which will end up with a 'partially build' meta database, without having a chance to call connected.add(sessionId);, so another context will start again building the meta database.

#2 Updated by Ovidiu Maxiniuc almost 3 years ago

I am not sure I understand. Please correct me if I am wrong.

We have an initial conversion over a .df which contains definition for a table X. As result, the X DMO interface is created and eventually compiled. At a later time, table X is dropped from the .df and a new incremental conversion is performed. As result, the X DMO interface is not created any more, but it is still present because nobody removes X.java (and eventually X.class).

When the application is build the old X.class will also packed inside the app.jar. As result when the server starts and meta database is populated, the DMO is found. Why a dump-name index collision would occur? The only possible case is when the X table is renamed to Y, but the dump-name attribute is kept, which will cause to new Y interface to carry the same dumpName attribute in @Table annotation.
In this case, assuming that this is not a 4GL error, I do not think we have the means to distinct at runtime which is the correct DMO interface Y or X which have the dumpName attribute of x.

I do not know what happens in case of temporary tables, but for permanent tables, I think we can dropped the DMOs even in case of an incremental conversion. The TRPL for (re-)generating the DMOs takes no more than a few minutes on a very large database. The most time consuming is the Metadata XML Generation, but this will be dropped soon, IIRC.

#3 Updated by Constantin Asofiei almost 3 years ago

Ovidiu, the scenario is this:
  • full conversion uses table Book with dump-name 'paper'.
  • incremental conversion removes table Book and adds a table Papyrus with dump-name 'paper'
  • the app uses these two tables only via dynamic queries, so the DMOs are not hard-references in the converted code.
  • FWD startup will fail after incremental conversion because there is Book.java and Papyrus.java, both with dump-name 'book'. The meta artifacts are OK (uses only Papyrus, as they are generated from conversion), but Book.java still exists and this will be picked up via Reflections usage. We do not want to force full conversion for such mistakes.

#4 Updated by Greg Shah almost 3 years ago

We don't support incremental conversion when the schema changes. This is expected to fail.

#5 Updated by Constantin Asofiei almost 3 years ago

The current solutions for a customer were:
  • delete all artifacts for this table manually (from src/, build/, other build folders, jar file with DMO classes)
  • having them run a full conversion which can takes days
The alternative would be to have a separate build mechanism for DMOs, which would:
  • keep the DMO files (and artifacts) in a src.dmo/ folder
  • be able to run this in standalone mode (via an incremental conversion mode, to keep the registry.xml AST IDs), and always clean src.dmo folder
  • compile would use a build/classes.dmo folder which gets properly cleaned up during build, to not have 'zombie' class files for removed DMOs
  • always jar the DMOs to a separate jar, and this jar gets removed before being re-created
  • customers can use their own build scripts and I don't know how easily the above can map to their build env

If these DMOs are DMOs which are used only via dynamic queries, then they do not have explicit references in the generated Java code, and the DMO class can be ignored safely by the FWD server, when registering the database.

The full solution is to make incremental conversion honor schema changes (#6083), but the build parts from the above (i.e. removing artifacts especially the .class file from the build jar), still apply - these can't be done by conversion, and need to be done by the build scripts.

IMO, having the FWD server be more lenient on 'zombie DMOs' can save time when working on configuring the source code for an application, to be converted in FWD.

#6 Updated by Ovidiu Maxiniuc almost 3 years ago

I think of the following solution:
  • add a conversion timestamp to DMOs (as an annotation). This might be useful for other issues.
  • when the runtime initializes the _meta database, especially _File and related, just ignore the DMOs which have an older timestamp.

I have only one unknown thing: at runtime, how do we decide which is the timestamp which marks the 'obsolete' DMOs?

#7 Updated by Constantin Asofiei almost 3 years ago

Ovidiu Maxiniuc wrote:

I have only one unknown thing: at runtime, how do we decide which is the timestamp which marks the 'obsolete' DMOs?

The schema .p2o/.schema/etc files will no longer contain the removed table, and will always be 'the latest' one; but the Java DMO can still exist - validating this DMO against the .p2o registry (or db.meta.xml, but we wanted to get rid of this?) will solve this. See DatabaseManager.getDatabaseDMOs, this list can be validated against the .p2o files for this schema. Same for getDatabaseTables.

I don't see how a timestamp at the DMO can help. Even after #6083 incremental conversion support, the .p2o will still always be the reference.

#8 Updated by Dănuț Filimon 11 months ago

  • Status changed from New to WIP
  • Assignee set to Dănuț Filimon

I'll pick this up since I worked on #6083.

#9 Updated by Dănuț Filimon 11 months ago

I tested hotel using two persistent tables (pt1 and pt2), after removing the pt2 table I am still left with those two files:
  • src/com/goldencode/hotel/dmo/hotel/Pt2.java
  • cvt/com/goldencode/hotel/dmo/hotel/Pt2.java.jast

It is possible to use the changes from 6083a to determine if a table (section) doesn't exist anymore, with this information we can just cleanup these files before we run the actual conversion.

#10 Updated by Dănuț Filimon 11 months ago

6083a does give me a way to determine the deleted tables, the only problem is getting the *.java and *.java.jast files and delete them. Looking into this right now.

#11 Updated by Greg Shah 11 months ago

I like the approach. You can put the changes into 6083a.

#12 Updated by Dănuț Filimon 11 months ago

  • Related to Feature #6083: schema (.df) changes in incremental conversion mode should only reconvert those programs that reference the changed schema added

#13 Updated by Dănuț Filimon 11 months ago

I tried to look into DatabaseManager.getDatabaseDMOs() and DatabaseManager.getDatabaseTables(), but the issue with those two is that they rely on the DIrectoryService which requires a BoostrapConfig.

#14 Updated by Dănuț Filimon 11 months ago

The dmo file name is build in java_dmo.xml and I can't build the file name of a dmo that doesn't exist. The only option is to save the filename of the dmo that was created in another table through ConversionData.

#15 Updated by Dănuț Filimon 11 months ago

Alexandru told me that import.sh is able to determine the DMO, I'll investigate and check if this can be of help.

#16 Updated by Dănuț Filimon 11 months ago

From the looks of it, I will need to load hotel.p2o which can give me access to the "path" annotation (com/goldencode/hotel/dmo/) and since I already have the schema name (hotel), I can built the path where I will find the DMO java file. Because I am loading the hotel.p2o, I can also find the table name based on the "historical" annotation ("table" doesn't match the actual table name from the df).

#17 Updated by Greg Shah 11 months ago

If possible, try not to make the changes for #7180 more difficult.

It also seems like a potentially good idea to store a list of all output artifacts (code and schema) in the conversion database, along with a mapping to their source artifact. With that, we could do the following:

  • Remove any output whose source artifact no longer exists. This is the subject of this task and of #6738. I'm suggesting that we may have a general solution to both problems.
  • Reconvert any source artifact whose output artifact has been deleted.

Constantin: What do you think about the 2nd one?

#18 Updated by Dănuț Filimon 11 months ago

We'd have to make a difference between code and schema artifacts, JavaPatternWorker.createJavaFile() is used for both and it's done much later in the conversion process. It is pretty difficult to match the correct schema table with the correct DMO java file.

#19 Updated by Greg Shah 11 months ago

It is fine to store at any point in the process, late or early, it should not matter.

#20 Updated by Dănuț Filimon 11 months ago

For #6738, I looked into the code and found JavaPatternWorker$WorkArea.pfiles, this helps us to track the persisted java asts and there's also PatternEngine.processAst() which takes a filename (ast name) which is the file being parsed. The issue is making them aware of each other as a starting point, which I am having trouble with. I can use the same finding for #7793, but this time I need to know which schema I am parsing (from the filename, or from loading the schema dict and checking the annotations) and which table is it for.

#21 Updated by Constantin Asofiei 11 months ago

Greg Shah wrote:

If possible, try not to make the changes for #7180 more difficult.

It also seems like a potentially good idea to store a list of all output artifacts (code and schema) in the conversion database, along with a mapping to their source artifact. With that, we could do the following:

  • Remove any output whose source artifact no longer exists. This is the subject of this task and of #6738. I'm suggesting that we may have a general solution to both problems.

The idea here is to remove .java, .jast, etc files if for example a table is deleted from the .df or, for #6738, a .p/.cls has been deleted? If so, there are also the .java/.jast files for temp-tables DMO if a temp-table gets removed from conversion. This is more complex as a temp-table doesn't have a single .java file, it also has super-interfaces which may or may not still be in use.

  • Reconvert any source artifact whose output artifact has been deleted.
I don't really understand the idea here. If a table gets deleted from .df, then we have two cases:
  • all 4GL code with static references for this table has already been changed and fixed, so the 4GL files will automatically be picked up by incremental conversion
  • some 4GL code with static references for this table still remains, and needs to be automatically reconverted (and it will fail parsing). But, if we remove the .java DMO for the table, then it will fail javac - this is a case where full rebuild will be required to remove .class files from the build directory, which no longer have a .java file.

#22 Updated by Greg Shah 11 months ago

  • Reconvert any source artifact whose output artifact has been deleted.
I don't really understand the idea here. If a table gets deleted from .df, then we have two cases:
  • all 4GL code with static references for this table has already been changed and fixed, so the 4GL files will automatically be picked up by incremental conversion
  • some 4GL code with static references for this table still remains, and needs to be automatically reconverted (and it will fail parsing). But, if we remove the .java DMO for the table, then it will fail javac - this is a case where full rebuild will be required to remove .class files from the build directory, which no longer have a .java file.

The idea here is about cases where an output artifact is deleted but the source artifact exists. For example, awesome.p is part of the application and it still exists, but Awesome.java was deleted by the developer. Should we identify this as a reason to reconvert awesome.p? It seems so because a conversion will be needed to have the result be complete. Instead of forcing a full conversion, we can just incrementally convert to get this output back.

#23 Updated by Constantin Asofiei 11 months ago

Greg Shah wrote:

The idea here is about cases where an output artifact is deleted but the source artifact exists. For example, awesome.p is part of the application and it still exists, but Awesome.java was deleted by the developer. Should we identify this as a reason to reconvert awesome.p? It seems so because a conversion will be needed to have the result be complete. Instead of forcing a full conversion, we can just incrementally convert to get this output back.

Thanks, makes sense then.

#24 Updated by Dănuț Filimon 11 months ago

I finally got something, the implementation works like this:
  • In ConversionData.java, the table_signature will also hold the filename (I worked it our so that it stores the java filename from the output folder), methods to update the filename and delete the table from table_signature.
  • In TransformDriver.java, the deleted tables are cleanup up before retrieving the modified tables.
  • In JavaPatternWorker.java, added storeDMOFilenameReference() that calls the update method for the table from ConversionData.
  • rules/schema/java_dmo.xml required changes to call the storeDMOFilenameReference()
  • Configuration.java, needed a method to get the conversion folder path from the output-root folder: getPathToConversionFolderFromOutputFolder
  • SchemaLoader.java, table cleanup method that deletes the java file and the java.jast file.

Everything works as expected! I need to do a cleanup, also the implementation did not take into account #6738 since I can take a simpler approach using file_signature.

#25 Updated by Dănuț Filimon 11 months ago

  • Status changed from WIP to Review
  • % Done changed from 0 to 100
  • reviewer Constantin Asofiei, Greg Shah added

I committed 6083a/16128. Added the changes mentioned in #7793-24.

Greg/Constantin, please review.

#26 Updated by Dănuț Filimon 11 months ago

There is an issue where the table should also be deleted from the registry.xml file. At a closer inspection I've found this in the registry.xml which is not good:

  <mapping id="51539607552" next="51539608533" treename="./cvt/com/goldencode/hotel/dmo/hotel/MetaSecAuthenticationDomain.java"/>
  <mapping id="55834574848" next="55834575387" treename="./cvt/com/goldencode/hotel/dmo/hotel/MetaSecAuthenticationSystem.java"/>
  <mapping id="60129542144" next="60129543934" treename="./cvt/com/goldencode/hotel/dmo/hotel/MetaUser.java"/>
  <mapping id="64424509440" next="64424509524" treename="./cvt/com/goldencode/hotel/dmo/hotel/Pt1.java"/>
  <mapping id="68719476736" next="68719477410" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaArea.java"/>
  <mapping id="73014444032" next="73014445695" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaConnect.java"/>
  <mapping id="77309411328" next="77309411826" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaDatabaseFeature.java"/>
  <mapping id="81604378624" next="81604379955" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaDb.java"/>
  <mapping id="85899345920" next="85899349430" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaField.java"/>
  <mapping id="90194313216" next="90194313754" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaFieldTrig.java"/>
  <mapping id="94489280512" next="94489284212" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaFile.java"/>
  <mapping id="98784247808" next="98784248227" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaFileTrig.java"/>
  <mapping id="103079215104" next="103079215583" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaFilelist.java"/>
  <mapping id="107374182400" next="107374184025" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaIndex.java"/>
  <mapping id="111669149696" next="111669150976" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaIndexField.java"/>
  <mapping id="115964116992" next="115964117887" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaLock.java"/>
  <mapping id="120259084288" next="120259084645" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaMyconnection.java"/>
  <mapping id="124554051584" next="124554052401" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaSequence.java"/>
  <mapping id="128849018880" next="128849020607" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaStartup.java"/>
  <mapping id="133143986176" next="133143986606" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaTablestat.java"/>
  <mapping id="137438953472" next="137438954201" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaTenant.java"/>
  <mapping id="141733920768" next="141733921455" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaTrans.java"/>
  <mapping id="146028888064" next="146028888751" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaUserio.java"/>
  <mapping id="150323855360" next="150323855894" treename="./cvt/com/goldencode/hotel/dmo/_meta/MetaUsertablestat.java"/>
  <mapping id="154618822656" next="154618823098" treename="./cvt/com/goldencode/hotel/dmo/_temp/FwdEmbeddedWindow_1.java"/>
  <mapping id="158913789952" next="158913790093" treename="./cvt/com/goldencode/hotel/dmo/_temp/PageToolbar_1.java"/>
  <mapping id="163208757248" next="163208757264" treename="./cvt/com/goldencode/hotel/dmo/_temp/FwdEmbeddedWindow_1_1.java"/>
  <mapping id="167503724544" next="167503724560" treename="./cvt/com/goldencode/hotel/dmo/_temp/PageToolbar_1_1.java"/>
All of those should have been .java.jast files!

#27 Updated by Dănuț Filimon 11 months ago

Made a fix in 6083a/16129 for the issue mentioned in #7793-26, the DMO will also be deleted from the registry.

#28 Updated by Greg Shah 11 months ago

Code Review Task Branch 6083a Revisions 16127 and 16128

Overall, it looks good.

1. In getPathToConversionFolderFromOutputFolder(), there is this code:

      String output = normalizeFilename(getParameter("output-root")) + File.separator;
      // we have to get the first directory from base, for example:
      // we have getParameter(basepath)="abl/src", we have to move the artifacts to "cvt/src" 
      String outputRoot = "." + File.separator;
      if (!output.equals(outputRoot))
      {
         String[] parts = output.split(Pattern.quote(File.separator));
         if (parts.length > 1)
         {
            // parts[0] will be ".", while parts[1] is the first directory of base
            outputRoot += parts[1] + File.separator;
         }
      }
      String cvtpath = DEFAULT_CVT;
      try
      {
         cvtpath = normalizeFilename(getConversionFolder()) + File.separator;
      }
      catch (ConfigurationException e)
      {
         LOG.log(Level.SEVERE, e.getMessage(), e);
      }

It seems like all of this can be done once and saved as static data.

2. It seems like the storeDMOFilename function is not really needed. Why not just call java.storeDMOFilenameReference() directly?

#29 Updated by Dănuț Filimon 11 months ago

I committed 6083a/16130 to address the #7793-28 review.

#30 Updated by Dănuț Filimon 11 months ago

Fixed a bug where the md5 field was initialized to null instead of filename: 6083a/16131.

#31 Updated by Greg Shah 11 months ago

  • Status changed from Review to Internal Test

Code Review Task Branch 6083a Revisions 16130 and 16131

The changes are good.

#32 Updated by Dănuț Filimon 11 months ago

The following should be tested:
  1. Conversion test, the .df should contain an additional table from the start.
  2. Incremental conversion test after modifying a table from the .df
  3. Incremental conversion test after deleting the table (check if the dependencies were removed: jast file, DMO java file, registry.xml)

The full test plan is available at https://proj.goldencode.com/projects/regression/wiki/Cross-Customer_Test_Plan_Tracking

#34 Updated by Greg Shah 10 months ago

  • Related to Bug #10639: Incremental conversion - unexpected DMO generation for space-only changes added

#35 Updated by Dănuț Filimon 9 months ago

  • version_resolved set to trunk/16220
  • Status changed from Internal Test to Test

Branch 6083a was merged to trunk revision 16220 and archived.

#36 Updated by Greg Shah 9 months ago

  • Status changed from Test to Closed

Also available in: Atom PDF