You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/03/19 01:25:44 UTC
[GitHub] [incubator-pinot] vincentchenjl opened a new pull request #5165:
[TE] Fix issue of displaying latests data in the speed-up case
vincentchenjl opened a new pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r395719318
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Sorry I didn't notice that.
Since here is the place all the windows are generated. I don't see a reason why you need to modify existing windows instead of adding the check when generating the windows.
Can we just change L289 from
DateTime endTime = monitoringEndTime.minus(windowDelayPeriod);
To:
DateTime windowEndTime = Math.min(monitoringEndTime.minus(windowDelayPeriod), endTime)?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] vincentchenjl commented on a change in pull
request #5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
vincentchenjl commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r395377027
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Yes, that's where I added the check.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] vincentchenjl commented on a change in pull
request #5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
vincentchenjl commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r396762683
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r396813007
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Great. Let's not fix for the symptom. Fix for the root cause, as early as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r395197812
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Why we generate the window larger than endTime?
Should we fix the window generation logic instead of patching here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r396004703
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
I got your point. Then we can do a little trick - modify endTime after using it.
How about the following? @vincentchenjl
`
List<Interval> monitoringWindows = new ArrayList<>();
List<DateTime> monitoringWindowEndTimes = getMonitoringWindowEndTimes();
DateTime detectionEndTime = new DateTime(endTime, dateTimeZone).minus(windowDelayPeriod);
for (DateTime monitoringEndTime : monitoringWindowEndTimes) {
DateTime endTime = monitoringEndTime.minus(windowDelayPeriod);
DateTime startTime = endTime.minus(windowSizePeriod);
endTime = endTime.isAfter(detectionEndTime) ? detectionEndTime : endTime;
monitoringWindows.add(new Interval(startTime, endTime));
}
`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r395375165
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Thanks for the change. I mean can we change getMonitoringWindows()?
This is the original place we generate the windows.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] vincentchenjl commented on a change in pull
request #5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
vincentchenjl commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r395908053
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
We cannot change the endTime here directly, as it can generate windows that overlaps the previous windows. Either we change the endTime of latest interval or we need to change the logic of generating the windows completely.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r396004703
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
I got your point. Then we can do a little trick - modify endTime after using it.
How about the following? @vincentchenjl
List<Interval> monitoringWindows = new ArrayList<>();
List<DateTime> monitoringWindowEndTimes = getMonitoringWindowEndTimes();
DateTime detectionEndTime = new DateTime(endTime, dateTimeZone).minus(windowDelayPeriod);
for (DateTime monitoringEndTime : monitoringWindowEndTimes) {
DateTime endTime = monitoringEndTime.minus(windowDelayPeriod);
DateTime startTime = endTime.minus(windowSizePeriod);
endTime = endTime.isAfter(detectionEndTime) ? detectionEndTime : endTime;
monitoringWindows.add(new Interval(startTime, endTime));
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r396002632
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Why it can generate windows that overlaps the previous windows? Can we sync offline?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] vincentchenjl commented on a change in pull
request #5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
vincentchenjl commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r395373844
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
Good point. Changed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun merged pull request #5165: [TE] Fix
issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun merged pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] xiaohui-sun commented on a change in pull request
#5165: [TE] Fix issue of displaying latests data in the speed-up case
Posted by GitBox <gi...@apache.org>.
xiaohui-sun commented on a change in pull request #5165: [TE] Fix issue of displaying latests data in the speed-up case
URL: https://github.com/apache/incubator-pinot/pull/5165#discussion_r396004703
##########
File path: thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/detection/wrapper/AnomalyDetectorWrapper.java
##########
@@ -290,6 +291,16 @@ long getLastTimeStamp() {
DateTime startTime = endTime.minus(windowSizePeriod);
monitoringWindows.add(new Interval(startTime, endTime));
}
+ if (isSpeedUp) {
Review comment:
I got your point. Then we can do a little trick - modify endTime after using it.
How about the following? @vincentchenjl
`List<Interval> monitoringWindows = new ArrayList<>();
List<DateTime> monitoringWindowEndTimes = getMonitoringWindowEndTimes();
DateTime detectionEndTime = new DateTime(endTime, dateTimeZone).minus(windowDelayPeriod);
for (DateTime monitoringEndTime : monitoringWindowEndTimes) {
DateTime endTime = monitoringEndTime.minus(windowDelayPeriod);
DateTime startTime = endTime.minus(windowSizePeriod);
endTime = endTime.isAfter(detectionEndTime) ? detectionEndTime : endTime;
monitoringWindows.add(new Interval(startTime, endTime));
}
`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org