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

feat: init metric engine structure #1554

Merged
merged 9 commits into from
Sep 11, 2024
Merged

Conversation

jiacai2050
Copy link
Contributor

@jiacai2050 jiacai2050 commented Aug 21, 2024

Rationale

See #1558

Detailed Changes

Add a new sub directory horaedb, all source codes for metric engine are under it.

Test Plan

Add a new ci.

@github-actions github-actions bot added the feature New feature or request label Aug 21, 2024
@jiacai2050 jiacai2050 force-pushed the feat-init-me branch 2 times, most recently from 729c11c to fdf93d4 Compare September 10, 2024 13:44

/// Time-aware merge storage interface.
#[async_trait]
pub trait TMStorage {
Copy link
Member

Choose a reason for hiding this comment

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

TM is confusing. How about writing as the full name TimeMergeStorage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


/// Time-aware merge storage interface.
#[async_trait]
pub trait TimeMergeStorage {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think may be just define the xxxRequest? And we fill it when we impl it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

Comment on lines 44 to 45
range: TimeRange,
predicates: Vec<Predicate>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems time range is a predicate.
And I think maybe we can just process the filter and projection by reusing realted fileds in old ReadRequest?
It seems not sure, may just define a empty ScanRequest in this sketch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems time range is a predicate.

TimeRange is special predicate, it must be included in every request.

And I think maybe we can just process the filter and projection by reusing realted fileds in old ReadRequest?

No code reuse for now.

It seems not sure, may just define a empty ScanRequest in this sketch.

The current one show basic usage, if we leave empty here, other dev have no idea what the query looks like.

horaedb/metric_engine/src/types.rs Outdated Show resolved Hide resolved
horaedb/metric_engine/src/storage.rs Show resolved Hide resolved
pub struct Manifest {}

impl Manifest {
pub fn new(id: u64) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

The id means?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UUID, it can be used to construct path for store it.

Comment on lines 20 to 23
pub struct SSTable {
pub sst_id: u64,
pub storage: ObjectStoreRef,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems storage should not be part of SSTable? The struct seems better to include the metadata(e.g. id, path)
Maybe we just define a empty struct in this sketch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will just leave an id in it.

Rachelint
Rachelint previously approved these changes Sep 11, 2024
Copy link
Contributor

@Rachelint Rachelint left a comment

Choose a reason for hiding this comment

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

LGTM

@ShiKaiWi
Copy link
Member

LGTM

@jiacai2050 jiacai2050 merged commit 7ccf97e into apache:main Sep 11, 2024
6 checks passed
@jiacai2050 jiacai2050 deleted the feat-init-me branch September 11, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants