akshayrai commented on a change in pull request #5991:
URL: https://github.com/apache/incubator-pinot/pull/5991#discussion_r486578318
##########
File path:
thirdeye/thirdeye-spi/src/main/java/org/apache/pinot/thirdeye/datalayer/pojo/AlertConfigBean.java
##########
@@ -211,7 +211,7 @@ public String toString() {
String properties;
public String getType() {
- if (StringUtil.isBlank(type)) {
+ if (isBlank(type)) {
Review comment:
You can use StringUtils from the `commons.lang3`?
##########
File path:
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/datalayer/dto/OnlineDetectionDataDTO.java
##########
@@ -1,6 +0,0 @@
-package org.apache.pinot.thirdeye.datalayer.dto;
Review comment:
Can you move these classes instead of deleting and recreating a new one?
##########
File path: thirdeye/pom.xml
##########
@@ -22,17 +22,18 @@
<modelVersion>4.0.0</modelVersion>
<groupId>org.apache.pinot.thirdeye</groupId>
<artifactId>thirdeye</artifactId>
- <version>${revision}${sha1}</version>
+ <version>0.1.0-SNAPSHOT</version>
<packaging>pom</packaging>
<name>thirdeye</name>
<modules>
<module>thirdeye-frontend</module>
+ <module>thirdeye-spi</module>
<module>thirdeye-pinot</module>
</modules>
<properties>
- <revision>1.0.0</revision>
+ <revision>0.1.0</revision>
Review comment:
Why lower the version? Shouldn't this be `2.0.0`?
##########
File path: thirdeye/pom.xml
##########
@@ -543,6 +550,17 @@
<artifactId>log4j-slf4j-impl</artifactId>
<version>${log4j2.version}</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
+ <version>${commons-lang3.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-math</artifactId>
+ <version>2.1</version>
+ </dependency>
Review comment:
Wouldn't it be better to define this at the respective levels? These are
not required across all the modules right!
##########
File path:
thirdeye/thirdeye-spi/src/main/java/org/apache/pinot/thirdeye/datalayer/dto/AlertSnapshotDTO.java
##########
@@ -80,48 +75,6 @@ public void setSnapshot(Multimap<String,
AnomalyNotifiedStatus> snapshot) {
}
}
- public void updateSnapshot(DateTime alertTime, List<MergedAnomalyResultDTO>
alertedAnomalies) {
Review comment:
Why move this?
##########
File path: thirdeye/pom.xml
##########
@@ -22,17 +22,18 @@
<modelVersion>4.0.0</modelVersion>
<groupId>org.apache.pinot.thirdeye</groupId>
<artifactId>thirdeye</artifactId>
- <version>${revision}${sha1}</version>
Review comment:
I think this is needed for the travis build!
+@vincentchenjl FYI
----------------------------------------------------------------
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:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
