Skip to content

Conversation

@aledsage
Copy link
Contributor

No description provided.

@geomacy
Copy link
Contributor

geomacy commented May 29, 2018

Copy link
Contributor

@geomacy geomacy left a comment

Choose a reason for hiding this comment

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

Looks good; just had a couple of minor thoughts

addReferencedObjects(prevDeltaCollector);
}

if (LOG.isTraceEnabled()) LOG.trace("Checkpointing delta of memento with references: "
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this LOG into the if above

this.persistPoliciesEnabled = BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_POLICY_PERSISTENCE_PROPERTY);
this.persistEnrichersEnabled = BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_ENRICHER_PERSISTENCE_PROPERTY);
this.persistFeedsEnabled = BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_FEED_PERSISTENCE_PROPERTY);
this.persistReferencedObjectsEnabled = BrooklynFeatureEnablement.isEnabled(BrooklynFeatureEnablement.FEATURE_REFERENCED_OBJECTS_PERSISTENCE_PROPERTY);
Copy link
Contributor

Choose a reason for hiding this comment

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

might be worth naming this rePersistReferencedObjectsEnabled as its behaviour is different from the flags above, isn't it - they disable persistence of feeds etc. entirely, whereas this just disables the periodic re-persistence. See comment on 542 below.

addReferencedObjects(instance);
}

private void addReferencedObjects(BrooklynObject instance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be called something different to distinguish it from the addReferencedObjects at 365, to highlight the different use? What do you think - addReferencedObjectsForInitialPersist or something?

@aledsage
Copy link
Contributor Author

Thanks @geomacy - good comments; all changes incorporated in an additional commit.

Copy link
Member

@tbouron tbouron left a comment

Choose a reason for hiding this comment

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

Review by @geomacy, comments addressed and LGTM. Thanks @aledsage.

Merging

@asfgit asfgit merged commit 49ee11d into apache:master Jun 1, 2018
asfgit pushed a commit that referenced this pull request Jun 1, 2018
@aledsage aledsage deleted the persistence-efficiency-do-less branch June 3, 2018 21:42
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.

4 participants