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

perf(cats): Deserialize relationships into Set vs List #2666

Merged
merged 2 commits into from
May 29, 2018

Conversation

ajordens
Copy link
Contributor

No description provided.

@ajordens ajordens requested a review from cfieber May 29, 2018 16:51
@@ -202,7 +202,7 @@ private boolean validType(String type) {
if (relationship == null) {
return new HashSet<>();
}
return relationship;
return relationship instanceof Set ? (Set<String>) relationship : new HashSet<>(relationship);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is probably not necessary as the deserialization is now to a Set but figured it was worth being explicit.

We have many implementations of CacheData and I did not immediately want to go down the path of changing constructors to move away from Collection<String> for relationships.

Reasonable?

Copy link
Contributor

@cfieber cfieber left a comment

Choose a reason for hiding this comment

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

lgtm

@ajordens ajordens merged commit 348d34c into spinnaker:master May 29, 2018
ajordens added a commit to ajordens/clouddriver that referenced this pull request May 30, 2018
Similar to spinnaker#2666 but this time behind a
`caching.redis.treatRelationshipsAsSet` flag.
ajordens added a commit to ajordens/clouddriver that referenced this pull request May 30, 2018
Similar to spinnaker#2666 but this time behind a
`caching.redis.treatRelationshipsAsSet` flag.
ajordens added a commit that referenced this pull request May 30, 2018
Similar to #2666 but this time behind a
`caching.redis.treatRelationshipsAsSet` flag.
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.

2 participants