-
Notifications
You must be signed in to change notification settings - Fork 920
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
Issue 8084: Decrease P3A upload interval. #4531
Conversation
@@ -43,7 +43,7 @@ constexpr char kLastRotationTimeStampPref[] = "p3a.last_rotation_timestamp"; | |||
|
|||
constexpr char kDefaultUploadServerUrl[] = "https://p3a.brave.com/"; | |||
|
|||
constexpr uint64_t kDefaultUploadIntervalSeconds = 60 * 60; // 1 hour. | |||
constexpr uint64_t kDefaultUploadIntervalSeconds = 60; // 1 minute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe we could do 1 * base::Time::kSecondsPerMinute
so that it's clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should transition all of the randomized delays from integral numbers of seconds to base::TimeDelta -- that's a more general issue that we should fix across the board.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@riastradh-brave probably, base::TimeDelta
, not base::Time
? Is there an issue for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry, wires crossed -- base::time::kSecondsPerMinute is an integer number of seconds; using base::TimeDelta itself is a different issue. Don't mind me!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
With the help of @linhkikuchi manually verified that CI is ok - only one unrelated build failure on mac |
Resolves brave/brave-browser#8084
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.