Fix issue with extra versions #1732

tst-lsavoie · 2020-05-04T20:12:30Z

This fixes two issues that cause extra versions to be produced unnecessarily during a build. The resulting database is correct, but the size of the database on disk is much larger than necessary.

The actual fixes are fairly small, but I did quite a bit of debugging and refactoring to track down the issue. Instead of throwing away the refactoring I am including it, so the changes are larger than one might expect. I tried to organize the changes into good commits, so it may be easier to review one commit at a time.

Here is a list of the major things I changed. I'll also provide comments on the PR to explain some of the changes.

Got rid of the "NoLongerNeeded" call that sometimes removed asset versions from the cache too early.
Re-copied the ReuseOrMakeAndUpdate and ReuseOrMakeAndUpdateSubasset code from previous versions into the Asset Factory. I'm not sure exactly what the difference is, but this fixes an issue that causes extra versions to be created.
Changed the template parameters for the functions in Asset Factory. The majority of lines changed relates to this. I created a new struct for every asset type that combines all the relevant related types for that asset and pass that as the template parameter to Asset Factory functions. This simplifies the code and makes it easier to get right, but it requires a lot of changes.
The above change required making significant changes to the unit tests. I also modified the tests to eliminate redundancies and achieve 100% coverage of Asset Factory.
Added some error checking to Storage Manager.
Changed how the Asset Serializer calculates filenames to match how the Storage Manager calculates filenames. I believe both methods were effectively identical, but this ensures there are no issues.

As a side effect of this fix, builds should be somewhat faster (due to building fewer versions of assets) and memory usage may be much less (due to extra versions of assets not hanging around in memory after they are removed from the cache). I've seen memory usage reductions of as much as 2/3 in system manager.

Fixes #1731

googlebot · 2020-05-05T20:20:25Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-05T20:21:06Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-05T20:25:14Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-05-05T20:27:29Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

tst-lsavoie · 2020-05-19T23:26:04Z

earth_enterprise/src/fusion/autoingest/AssetFactory.h

-
-  template <class MutableDerivedVersionHandleType,
-            class ConfigType, class VersionDType>
-  MutableDerivedVersionHandleType ReuseOrMakeAndUpdate(const std::string& ref_,


I copied over all the ReuseOrMakeAndUpdate* functions from earlier versions, which was the fix to one of the issues. Unfortunately it makes the diff hard to read.

tst-lsavoie · 2020-05-19T23:26:20Z

earth_enterprise/src/fusion/autoingest/AssetFactory.h

-                  }
-                  else
-                  {
-                      version.NoLongerNeeded();


Removing the NoLongerNeeded calls was a fix to one of the bugs.

tst-lsavoie · 2020-05-19T23:27:00Z

earth_enterprise/src/fusion/autoingest/AssetFactory.h

@@ -49,210 +55,181 @@ namespace AssetFactory
      return nullptr;
  }

-  template<class AssetType>


Most of the changes in this file are to use new template parameters that make the code clearer and make it easier to use the right template parameters. I also deleted a few functions that weren't used.

tst-lsavoie · 2020-05-19T23:28:47Z

earth_enterprise/src/fusion/autoingest/AssetFactory_unittest.cpp

@@ -27,49 +28,13 @@
 using namespace std;
 using namespace AssetFactory;

-// forward declare classes


I essentially rewrote the MockAsset class hierarchy from scratch. I tried to keep parts from the original version but that probably won't show up in the diff. The tests themselves should be more similar, but I did make some changes, including getting rid of duplicate tests and adding new tests to get better coverage.

tst-lsavoie · 2020-05-19T23:30:21Z

earth_enterprise/src/fusion/autoingest/AssetFactory.h

  }

-  template<class VersionType>
+  template<class AssetType>
+  inline typename AssetType::VersionD FindVersion(const std::string & ref, const AssetDefs::Type & type) {


The Find function is a special case since it can find assets or asset versions, but the new template parameter includes both assets and versions in the same struct. I got around that by creating a FindVersion function so the regular Find function can always return an asset. The FindVersion function is only used below in this file.

tst-lsavoie · 2020-05-19T23:33:15Z

earth_enterprise/src/fusion/autoingest/AssetGenDaemonH.pl

@@ -283,6 +275,20 @@ ()

 print $fh <<EOF;

+// Convenience class that ties together related types


This struct puts all the classes that relate to a specific asset type into one place where it's easy to reference several of them at once. It makes the Asset Factory functions much cleaner, but it requires a lot of changes to use it.

This makes the template parameters a LOT cleaner.

tst-jlarocco · 2020-05-26T20:55:35Z

I didn't test, but the code looks good to me.

I don't see where the bug was in ReuseOrMakeAndUpdate, either... It looks nearly identical.

* Move ReuseOrMakeAndUpdate out of the asset factory * Don't remove assets from the cache unnecessarily * Improved error handling in Storage Manager * Simply asset factory templates and remove unused functions * Move ReuseOrMakeAndUpdate back to the asset factory * Clean up unnecessary namespace references * Improve test coverage

googlebot added the cla: yes Manual verification that all contributors have signed the CLA. label May 4, 2020

tst-lsavoie marked this pull request as draft May 4, 2020 20:13

tst-lsavoie force-pushed the extra-versions branch from ef88c5b to 71b7af8 Compare May 5, 2020 20:20

googlebot added cla: no and removed cla: yes Manual verification that all contributors have signed the CLA. labels May 5, 2020

tst-lsavoie force-pushed the extra-versions branch from 71b7af8 to ef88c5b Compare May 5, 2020 20:21

googlebot added cla: yes Manual verification that all contributors have signed the CLA. and removed cla: no labels May 5, 2020

tst-lsavoie force-pushed the extra-versions branch from ef88c5b to 897f7ba Compare May 5, 2020 20:25

googlebot added cla: no and removed cla: yes Manual verification that all contributors have signed the CLA. labels May 5, 2020

tst-lsavoie changed the base branch from master to release_5.3.4.1 May 5, 2020 20:25

googlebot added cla: yes Manual verification that all contributors have signed the CLA. and removed cla: no labels May 5, 2020

tst-lsavoie marked this pull request as ready for review May 5, 2020 20:28

Move ReuseOrMakeAndUpdate out of the asset factory

8170ef1

tst-lsavoie force-pushed the extra-versions branch from 6059ee0 to 8170ef1 Compare May 19, 2020 15:07

tst-lsavoie added 5 commits May 19, 2020 15:57

Don't remove assets from the cache unnecessarily

499068a

Improved error handling in Storage Manager

cb5fd0d

Simply asset factory templates and remove unused functions

87d88af

Move ReuseOrMakeAndUpdate back to the asset factory

de4ddfa

Clean up unnecessary namespace references

661d721

tst-lsavoie changed the title ~~WIP: Fix issue with extra versions~~ Fix issue with extra versions May 19, 2020

Improve test coverage

721cf3f

tst-lsavoie force-pushed the extra-versions branch from d09c1ec to 721cf3f Compare May 19, 2020 23:20

tst-lsavoie commented May 19, 2020

View reviewed changes

tst-jlarocco approved these changes May 26, 2020

View reviewed changes

tst-jkent approved these changes May 29, 2020

View reviewed changes

tst-jkent merged commit 0852291 into google:release_5.3.4.1 May 29, 2020

tst-lsavoie deleted the extra-versions branch June 25, 2020 15:57

tst-lsavoie mentioned this pull request Jun 29, 2020

Historical builds create extra versions of assets #1731

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with extra versions #1732

Fix issue with extra versions #1732

tst-lsavoie commented May 4, 2020 •

edited

googlebot commented May 5, 2020

googlebot commented May 5, 2020

googlebot commented May 5, 2020

googlebot commented May 5, 2020

tst-lsavoie May 19, 2020

tst-lsavoie May 19, 2020

tst-lsavoie May 19, 2020 •

edited

tst-lsavoie May 19, 2020

tst-lsavoie May 19, 2020

tst-lsavoie May 19, 2020

tst-jlarocco May 26, 2020

tst-jlarocco commented May 26, 2020 •

edited

		@@ -283,6 +275,20 @@ ()

		print $fh <<EOF;

		// Convenience class that ties together related types

Fix issue with extra versions #1732

Fix issue with extra versions #1732

Conversation

tst-lsavoie commented May 4, 2020 • edited

googlebot commented May 5, 2020

googlebot commented May 5, 2020

googlebot commented May 5, 2020

googlebot commented May 5, 2020

tst-lsavoie May 19, 2020

Choose a reason for hiding this comment

tst-lsavoie May 19, 2020

Choose a reason for hiding this comment

tst-lsavoie May 19, 2020 • edited

Choose a reason for hiding this comment

tst-lsavoie May 19, 2020

Choose a reason for hiding this comment

tst-lsavoie May 19, 2020

Choose a reason for hiding this comment

tst-lsavoie May 19, 2020

Choose a reason for hiding this comment

tst-jlarocco May 26, 2020

Choose a reason for hiding this comment

tst-jlarocco commented May 26, 2020 • edited

tst-lsavoie commented May 4, 2020 •

edited

tst-lsavoie May 19, 2020 •

edited

tst-jlarocco commented May 26, 2020 •

edited