Skip to content

Conversation

@platoneko
Copy link
Contributor

@platoneko platoneko commented Aug 2, 2022

Proposed changes

Issue Number: close #xxx

Problem summary

Describe your changes.

Checklist(Required)

  1. Does it affect the original behavior:
    • Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • No
    • No Need
  3. Has document been added or modified:
    • Yes
    • No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

.add("TabletId").add("ReplicaId").add("BackendId").add("SchemaHash").add("Version")
.add("LstSuccessVersion").add("LstFailedVersion").add("LstFailedTime")
.add("DataSize").add("RowCount").add("State")
.add("LocalDataSize").add("RemoteDataSize").add("RowCount").add("State")
Copy link
Contributor

Choose a reason for hiding this comment

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

This would cause compatibility problems, would using DataSize also work?

Copy link
Contributor Author

@platoneko platoneko Aug 4, 2022

Choose a reason for hiding this comment

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

Of course we can use DataSize here, but I think it may be a little confusing when users find DataSize is 0.

@platoneko platoneko force-pushed the show_remote_data_size branch 4 times, most recently from 16f81b5 to f4a8caf Compare August 8, 2022 12:24
@platoneko platoneko changed the title [feature](cold_on_s3) Show remote data usage [feature](cold_on_s3) Show remote data usage via SHOW PROC and SHOW TABLETS statements Aug 11, 2022
@platoneko platoneko force-pushed the show_remote_data_size branch from f4a8caf to 1481631 Compare August 11, 2022 10:31
@platoneko platoneko changed the title [feature](cold_on_s3) Show remote data usage via SHOW PROC and SHOW TABLETS statements [feature](cold_on_s3) Show remote data usage via SHOW BACKENDS and SHOW TABLETS statements Aug 11, 2022
@platoneko platoneko force-pushed the show_remote_data_size branch from 1481631 to 82e2c5d Compare August 11, 2022 12:41
auto key = get_key(path);
auto fs_path = Path(_s3_conf.endpoint) / _s3_conf.bucket / key;
*reader = std::make_unique<S3FileReader>(std::move(fs_path), fsize, std::move(key),
*reader = std::make_shared<S3FileReader>(std::move(fs_path), fsize, std::move(key),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to shared ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a typo(not bug) fix, as function signature is S3FileSystem::open_file(const Path& path, FileReaderSPtr* reader)

.add("SystemDecommissioned").add("ClusterDecommissioned").add("TabletNum")
.add("DataUsedCapacity").add("AvailCapacity").add("TotalCapacity").add("UsedPct")
.add("MaxDiskUsedPct").add("Tag").add("ErrMsg").add("Version").add("Status")
.add("MaxDiskUsedPct").add("Tag").add("ErrMsg").add("Version").add("Status").add("RemoteUsedCapacity")
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to add RemoteUsedCapacity right after DataUsedCapacity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some unit tests and regression tests rely on the subscript of these data. If RemoteUsedCapacity is inserted into the middle, the tests will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

public static final ImmutableList<String> TITLE_NAMES = new ImmutableList.Builder<String>().add("ReplicaId")
.add("BackendId").add("Version").add("LstSuccessVersion").add("LstFailedVersion").add("LstFailedTime")
.add("SchemaHash").add("LocalDataSize").add("RowCount").add("State").add("IsBad")
.add("VersionCount").add("PathHash").add("MetaUrl").add("CompactionStatus").add("RemoteDataSize").build();
Copy link
Contributor

Choose a reason for hiding this comment

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

Add RemoteDataSize right after LocalDataSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.add("LocalDataSize").add("RowCount").add("State")
.add("LstConsistencyCheckTime").add("CheckVersion")
.add("VersionCount").add("PathHash").add("MetaUrl").add("CompactionStatus")
.add("VersionCount").add("PathHash").add("MetaUrl").add("CompactionStatus").add("RemoteDataSize")
Copy link
Contributor

Choose a reason for hiding this comment

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

after LocalDataSize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@morningman morningman added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Aug 20, 2022
@platoneko platoneko force-pushed the show_remote_data_size branch from 82e2c5d to 97f7d09 Compare August 20, 2022 13:57
morningman
morningman previously approved these changes Aug 20, 2022
Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Aug 20, 2022
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@platoneko platoneko force-pushed the show_remote_data_size branch from 97f7d09 to dd01403 Compare August 24, 2022 03:29
@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Aug 24, 2022
@platoneko platoneko force-pushed the show_remote_data_size branch from dd01403 to 8d673db Compare August 25, 2022 02:39
@morningman morningman merged commit 588dc5f into apache:master Aug 25, 2022
@morningman morningman mentioned this pull request Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/test reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants