Skip to content
This repository has been archived by the owner on Oct 17, 2018. It is now read-only.

JSON marshaling of policies list #59

Merged
merged 3 commits into from
Jun 21, 2017

Conversation

xichen2020
Copy link
Contributor

cc @cw9 @prateek @jeromefroe

This PR adds logic to perform JSON marshalling/unmarshaling of a list of policies.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 89.199% when pulling 892f023 on xichen-json-marshaling-policieslist into 0ba2334 on master.

Copy link
Contributor

@cw9 cw9 left a comment

Choose a reason for hiding this comment

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

Where do we need the JSON marshaling?

false,
[]Policy{
NewPolicy(NewStoragePolicy(10*time.Second, xtime.Second, 6*time.Hour), DefaultAggregationID),
NewPolicy(NewStoragePolicy(time.Minute, xtime.Minute, 24*time.Hour), mustCompress(Lower, Upper)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do mustCompress(Count, Mean) to avoid having to change Lower and Upper to Min and Max later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sounds good.


// stagedPoliciesJSON is used for marshaling and unmarshaling staged policies.
type stagedPoliciesJSON struct {
CutoverNanos int64 `json:"cutoverNanos"`
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just define the json properties in the original StagedPolicies struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately JSON marshaler doesn't marshal the unexported field policies in staged policies.

@xichen2020
Copy link
Contributor Author

@cw9 To persist policies list in our index.

@cw9
Copy link
Contributor

cw9 commented Jun 20, 2017

As discussed offline, we also need to store version of the policiesList

Copy link
Contributor

@cw9 cw9 left a comment

Choose a reason for hiding this comment

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

LGTM once unit test is updated

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.07%) to 88.007% when pulling 657d3df on xichen-json-marshaling-policieslist into 0ba2334 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 89.173% when pulling 657d3df on xichen-json-marshaling-policieslist into 0ba2334 on master.

@xichen2020 xichen2020 merged commit 53e3b67 into master Jun 21, 2017
@xichen2020 xichen2020 deleted the xichen-json-marshaling-policieslist branch June 21, 2017 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants