Skip to content

Fix TTL issue.#3292

Merged
wu-sheng merged 4 commits intomasterfrom
record-ttl
Aug 20, 2019
Merged

Fix TTL issue.#3292
wu-sheng merged 4 commits intomasterfrom
record-ttl

Conversation

@wu-sheng
Copy link
Copy Markdown
Member

Description in #3263, I am adding new TTL Calculators for record type, in ES and MySQL storages.

W/ this, the TTL should work well in delete record.

@wu-sheng wu-sheng added bug Something isn't working and you are sure it's a bug! core feature Core and important feature. Sometimes, break backwards compatibility. backend OAP backend related. labels Aug 20, 2019
@wu-sheng wu-sheng added this to the 6.4.0 milestone Aug 20, 2019
kezhenxu94
kezhenxu94 previously approved these changes Aug 20, 2019
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

LGTM

public class RecordTTLCalculator implements TTLCalculator {

@Override public long timeBefore(DateTime currentTime, DataTTLConfig dataTTLConfig) {
return Long.valueOf(currentTime.plusMinutes(0 - dataTTLConfig.getRecordDataTTL()).toString("yyyyMMddHHmm"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Segment time bucket is the second time format.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed, w/ wrong in EsRecordTTLCalculator too, we should use #plusDays there, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Alarm record time bucket is also the second time format.

Alarm

record.setStartTime(message.getStartTime());
record.setTimeBucket(TimeBucket.getSecondTimeBucket(message.getStartTime()));

Segment

long timeBucket = TimeBucket.getSecondTimeBucket(segmentCoreInfo.getStartTime());
segment.setTimeBucket(timeBucket);

Suggest that, rename the getSecondTimeBucket to getRecordTimeBucket or create an alias name of getRecordTimeBucket, it will make it clear.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed, w/ wrong in EsRecordTTLCalculator too, we should use #plusDays there, right?

En, En.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toString("yyyyMMddHHmm") is wrong, modify to toString("yyyyMMddHHmmss").

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Plus minute is for the config item in application.yml.
recordDataTTL: ${SW_CORE_RECORD_DATA_TTL:90} # Unit is minute

toString("yyyyMMddHHmmss") if for the database, the timeBucket column in database is second data format.

@wu-sheng
Copy link
Copy Markdown
Member Author

@peng-yongsheng Method rename done.

public class RecordTTLCalculator implements TTLCalculator {

@Override public long timeBefore(DateTime currentTime, DataTTLConfig dataTTLConfig) {
return Long.valueOf(currentTime.plusMinutes(0 - dataTTLConfig.getRecordDataTTL()).toString("yyyyMMddHHmm"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

toString("yyyyMMddHHmm") is wrong, modify to toString("yyyyMMddHHmmss").

@wu-sheng
Copy link
Copy Markdown
Member Author

@peng-yongsheng Have you refreshed the page? You are referring the Outdated codes.

Copy link
Copy Markdown
Member

@peng-yongsheng peng-yongsheng left a comment

Choose a reason for hiding this comment

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

^_^. ... Nothing to say.

@wu-sheng wu-sheng merged commit dc0f653 into master Aug 20, 2019
@wu-sheng wu-sheng deleted the record-ttl branch August 20, 2019 07:36
@wu-sheng wu-sheng mentioned this pull request Aug 26, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related. bug Something isn't working and you are sure it's a bug! core feature Core and important feature. Sometimes, break backwards compatibility.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants