Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite synchronizer deletion and add locking changes in runner #55

Merged
merged 8 commits into from Jul 27, 2022

Conversation

jackson-at-bentley
Copy link
Contributor

@jackson-at-bentley jackson-at-bentley commented Jul 15, 2022

This PR contains the two changes from Sam's email feedback.

  1. Instead of using the synchronizer's cache to find elements that are scoped to a given repository link, we do a postorder traversal of the iModel starting at the root subject and delete elements we haven't seen and empty models. My hope is that the ExternalSourceAspect's are cleaned up because of their embedding relationship (Figure 1). I don't think the repository links and external sources are cleaned up.
  2. The runner has been rewritten to use the new locking API.

Unfortunately the two tests in #standalone and #integration that make an update pass with the synchronizer are now both failing because the iModel is shredded by the new deletion algorithm. Fixed. The connector observed that the source file is unchanged, and skipped the second pass over the iModel.


Okay, this marks the start of a bit of a rant. Hopefully it reflects an incorrect understanding of the framework.

In my opinion, the TestConnector indicates a larger problem with the synchronizer's current API.

  • It seems to operate under the philosophy that the connector author is responsible for the intermediate representation of the iModel, but requires them to feed it SynchronizationResults, which are trees of elements. All of the children of such a tree do not have provenance information, and the synchronizer ignores their change state. If it detects that the parent has changed, it will fall into a recursive update of the children (updateResultsInIModelForChildren seems to be buggy as well).
  • An update requires the iModel ID of the element, not the identifier that anchors it to the source document in its ExternalSourceAspect. The author shouldn't be responsible for providing that ID, because we have the element's provenance information.
  • I think as Zach noticed, the synchronizer goes only halfway towards defining an intermediate representation and ignores models.

Like Jeff said, the synchronizer is the minimal set of tools needed to map a source file to an iModel. It's super important. I think it needs a new API. We don't have to be as opinionated as PCF (defining loaders, relationship nodes), but I think we should move closer towards it and provide an intermediate representation. We already have such objects: they're defined in iTwin. With a bit of declaration merging in TypeScript, we can present the synchronizer with element properties and provenance and let it do its thing.

Something like this API. The difficulty is in replacing references to iModel IDs with provenance.

function sync<E extends Element, M extends Model>(tree: E | M): void;
function prune<E extends Element, M extends Model>(tree: E | M): void;

Figure 1

A diagram of the elements of an iModel for which the synchronizer is reponsible

src/Synchronizer.ts Show resolved Hide resolved
src/ConnectorRunner.ts Outdated Show resolved Hide resolved
src/ConnectorRunner.ts Outdated Show resolved Hide resolved
src/Synchronizer.ts Show resolved Hide resolved
@jackson-at-bentley
Copy link
Contributor Author

A few concerns:

  • Are there any other embedding relationships or dependencies that will cause the SQLite database to buck us during the traversal?
  • Cleaning up may take multiple runs, for example, if a geometry stream depends on a definition element and the geometry stream is visited first.
  • Deleting definition elements is really expensive and shouldn't be done individually.

@jchick-bentley jchick-bentley merged commit e828da1 into main Jul 27, 2022
@jchick-bentley jchick-bentley deleted the atomics branch July 27, 2022 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants