Skip to content
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

remove serde.retention, use retention.max-hr * hour_seconds #56

Merged
merged 3 commits into from
Dec 14, 2020

Conversation

chenqin
Copy link
Contributor

@chenqin chenqin commented Dec 14, 2020

  • group max-mb max-hr under retention configuration
  • remove ser.retention use max-hr double value multiple with 3600 as kafka ingestion retention config

@chenqin chenqin changed the title remove ser.retention, use retention.max-hr * hour_seconds remove serde.retention, use retention.max-hr * hour_seconds Dec 14, 2020
Copy link
Collaborator

@shawncao shawncao left a comment

Choose a reason for hiding this comment

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

I think the change in for Kafka serde is inappropriate, otherwise looks good!

auto retention = node["retention"];
if (retention) {
serde.retention = retention.as<uint64_t>();
}
serde.retention = (uint64_t) retention;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This retention is KAFKA topic retention in Kafka store, not the max-hr (retention in Nebula), the usage of this value is used to estimate offset and range for each Kafka split when early version of Kafka doesn't have watermark/offset feature, we should leave this as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, how about rename to topic-retention to avoid confusion.

src/meta/ClusterInfo.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@shawncao shawncao left a comment

Choose a reason for hiding this comment

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

sounds good to me, thanks for the update.

@chenqin
Copy link
Contributor Author

chenqin commented Dec 14, 2020

Notice to upgrade table compatible with this change
max-mb -> retention.max-mb
max-hr -> retention.max-hr
serde.retention -> serde.topic-retention

@chenqin chenqin merged commit b64a2b6 into varchar-io:master Dec 14, 2020
@chenqin chenqin deleted the retention branch December 14, 2020 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants