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
[TE] clean up decprecated/unused code #5435
Conversation
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.
-8215, that's some serious cleaning.
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.
Great progress. @kishoreg FYI
...irdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/ThirdEyeAnomalyApplication.java
Outdated
Show resolved
Hide resolved
...irdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/ThirdEyeAnomalyApplication.java
Outdated
Show resolved
Hide resolved
...irdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/ThirdEyeAnomalyApplication.java
Outdated
Show resolved
Hide resolved
...eye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/task/TaskInfoFactory.java
Show resolved
Hide resolved
...e/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/task/TaskRunnerFactory.java
Show resolved
Hide resolved
@@ -261,13 +261,4 @@ public static DateTimeFormatter getDateTimeFormatterForDataset(TimeSpec timeSpec | |||
return bucketNameToCountStar; | |||
} | |||
|
|||
public static double getPercentCompleteness(PercentCompletenessFunctionInput input) { |
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.
This whole class could be deleted.
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.
This class is used in Wo4WAvgDataCompletenessAlgorithm
. We need to drop both of them if we want to drop this. Wo4WAvgDataCompletenessAlgorithm
cannot be dropped due to the issue below.
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 remove it and also remove getter/setter.
That should be safe.
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.
I remove Wo4WAvgDataCompletenessAlgorithm
completely and also the field DatasetConfigBean.dataCompletenessAlgorithm
from DatasetConfigBean
.
...in/java/org/apache/pinot/thirdeye/completeness/checker/Wo4WAvgDataCompletenessAlgorithm.java
Outdated
Show resolved
Hide resolved
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.
Nice work. Thank you for keeping the code base healthy!
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.
Nice diligent effort!
Make sure to remove any traces in db and config if any.
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/task/TaskConstants.java
Outdated
Show resolved
Hide resolved
...eye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/task/TaskInfoFactory.java
Outdated
Show resolved
Hide resolved
thirdeye/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/anomaly/task/TaskConstants.java
Outdated
Show resolved
Hide resolved
...e/thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/common/BaseThirdEyeApplication.java
Outdated
Show resolved
Hide resolved
@@ -160,14 +155,12 @@ public void run(ThirdEyeDashboardConfiguration config, Environment env) | |||
|
|||
AnomalyFunctionFactory anomalyFunctionFactory = new AnomalyFunctionFactory(config.getFunctionConfigPath()); |
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.
AnomalyFunctionFactory is not used.
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.
anomalyFunctionFactory
is used to initialize AnomalyResource
, DataResource
, and AnomaliesResource
. Could we confirm all these three resources are not used?
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.
@jihaozh can you check which resource is used? I believe most endpoints in AnomalyResource and AnomaliesResource are not used. From Harley's doc only "/anomalies/search/time/" is used, but let's double check with them.
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.
I think AnomalyResource
is not used. In AnomaliesResource
, only the search endpoints listed by Harley is used. DataResource
is being used, but it shouldn't need anomalyFunctionFactory
anymore.
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.
When we search for anomalies by using /anomalies/search/time/
, we construct AnomalyDetails
, and it requires BaseAnomalyFunction
to do it.
private DatasetConfigManager datasetConfigDAO; | ||
private AnomalyFunctionFactory anomalyFunctionFactory; | ||
private AlertFilterFactory alertFilterFactory; | ||
private LoadingCache<String, Long> collectionMaxDataTimeCache; | ||
|
||
private static final DAORegistry DAO_REGISTRY = DAORegistry.getInstance(); | ||
|
||
public AnomalyResource(AnomalyFunctionFactory anomalyFunctionFactory, AlertFilterFactory alertFilterFactory, | ||
AlertFilterAutotuneFactory alertFilterAutotuneFactory) { | ||
public AnomalyResource(AnomalyFunctionFactory anomalyFunctionFactory, AlertFilterFactory alertFilterFactory) { | ||
this.anomalyFunctionDAO = DAO_REGISTRY.getAnomalyFunctionDAO(); | ||
this.anomalyMergedResultDAO = DAO_REGISTRY.getMergedAnomalyResultDAO(); |
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.
What is the difference between anomalyMergedResultDAO and mergedAnomalyResultDAO. Seems duplicate.
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.
Both these two are used in the class. If we r sure AnomalyResource
is not used, we can remove the whole class.
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.
The question is duplication. We should remove one of them.
...rdeye-pinot/src/main/java/org/apache/pinot/thirdeye/dashboard/resources/AnomalyResource.java
Outdated
Show resolved
Hide resolved
@@ -101,21 +99,18 @@ | |||
private MergedAnomalyResultManager anomalyMergedResultDAO; | |||
private AlertConfigManager emailConfigurationDAO; | |||
private MergedAnomalyResultManager mergedAnomalyResultDAO; | |||
private AutotuneConfigManager autotuneConfigDAO; | |||
private DatasetConfigManager datasetConfigDAO; | |||
private AnomalyFunctionFactory anomalyFunctionFactory; | |||
private AlertFilterFactory alertFilterFactory; |
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.
Not used.
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.
It is used in line 141.
anomalyResults = AlertFilterHelper.applyFiltrationRule(anomalyResults, alertFilterFactory);
...thirdeye-pinot/src/main/java/org/apache/pinot/thirdeye/datalayer/pojo/DatasetConfigBean.java
Outdated
Show resolved
Hide resolved
autotuneConfigDTO.setPerformance(performance); | ||
return autotuneConfigDTO; | ||
} | ||
|
||
public static ClassificationConfigDTO getTestClassificationConfig(String name, List<Long> mainFunctionIdList, |
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.
Not used.
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.
This can be removed in next PR with all the unused DAO objects.
@cecilynie @cyenjung FYI. |
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. Thanks for cleaning it up.
Clean up deprecated code
Refactor a bit to remove class dependency