-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Index management] Data stream edit data retention #167006
[Index management] Data stream edit data retention #167006
Conversation
@elasticmachine merge upstream |
merge conflict between base and head |
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
@elasticmachine merge upstream |
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.
Really great work @sabarasaba! Thanks for the quick turnaround on this. I tried to test all the different combinations and everything worked as expected (with the known usability issues) - DSL enabled, DSL disabled, DSL enabled with predefined retention, ILM + DSL enabled. Left some comments in the code.
Also, a few other observations:
- I noticed the bug you mentioned to me about "Indices" sometimes showing twice in the flyout. This indeed appears to be a regression of [Index Management] Support data retention on Data Streams tab #165263. I think in
data_stream_detail_panel.tsx
, if you change L79 to as follows it should fix it:
const descriptionListColumnTwo = descriptionListItems.slice(midpoint, descriptionListItems.length);
- [Follow up] If a user selects "Never delete data" and saves, I think we should represent this in the table view as well (right now this translates as an empty table cell for the "Data retention" column). This is probably more appropriate as a follow-up PR, but just something I noticed and wanted to call out.
- [Follow up] We might want to consider creating human readable units to display in the table. For example
7m
could be interpreted as "minutes" or "months". Again, not directly related to your work, just something I noticed.
x-pack/plugins/index_management/__jest__/client_integration/home/data_streams_tab.helpers.ts
Show resolved
Hide resolved
...ication/sections/home/data_stream_list/data_stream_detail_panel/data_stream_detail_panel.tsx
Show resolved
Hide resolved
...ication/sections/home/data_stream_list/data_stream_detail_panel/data_stream_detail_panel.tsx
Outdated
Show resolved
Hide resolved
...ication/sections/home/data_stream_list/data_stream_detail_panel/data_stream_detail_panel.tsx
Show resolved
Hide resolved
...t/public/application/sections/home/data_stream_list/edit_data_retention_modal/unit_field.tsx
Outdated
Show resolved
Hide resolved
...t/public/application/sections/home/data_stream_list/edit_data_retention_modal/unit_field.tsx
Outdated
Show resolved
Hide resolved
x-pack/test/api_integration/apis/management/index_management/data_streams.ts
Show resolved
Hide resolved
x-pack/test/api_integration/apis/management/index_management/data_streams.ts
Show resolved
Hide resolved
Hey @sabarasaba, I'll review the copy today on behalf of @gchaps. |
@elasticmachine merge upstream |
Thanks for the review @alisonelizabeth! I've addressed your feedback with 59ba5f6 and created a new issue for dealing with the two issues you mentioned above about being more explicit about data retention period in the UI. |
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.
Hey @sabarasaba. I've suggested a few edits. Also have a few questions/remarks for things I'm not sure of:
-
I was not sure of what to suggest for the warning, but I think we can make it clearer. See my comments in the review
-
Another thing that caught my attention are the shortened time values we use.
- I understand that they match with the API but "nanos" "micros" etc. sound quite unusual to me compared to common abbreviations (d, h, min, sec, ms, µs, ns) or fully developed forms if we have enough space for it, like
3000 microseconds
- This maybe also has to do with the API but we render the value with no space in the flyout, while the best practice would be to have a (non-breaking) space between the number and the unit. (30 ns, 4 days, instead of 30ns, 4days). I see it's similar for the storage size so at least it's consistent, but if we can consider changing this at some point in the UI, it would be great.
- I understand that they match with the API but "nanos" "micros" etc. sound quite unusual to me compared to common abbreviations (d, h, min, sec, ms, µs, ns) or fully developed forms if we have enough space for it, like
- Tooltips for "Data retention" differ in the flyout and in the data stream list. I'm not sure if they are still correct, especially the long one? Happy to help revisit them once I'm a bit more sure about the actual behavior ^^'
- Not a content comment, but is it the expected behavior that once we edit the data retention, the flyout also closes?
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
Outdated
Show resolved
Hide resolved
Thanks for the review @florent-leborgne! I've addressed some of your feedback with 0034336 and I've created this issue to address some of the other smaller things you mentioned. In regards of the tooltip content, I'm not sure if thats a regresion or not cc: @alisonelizabeth. |
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.
Approving latest copy while noting that pending comments will be addressed through separate issues as discussed. Thanks!
x-pack/plugins/index_management/__jest__/client_integration/helpers/test_subjects.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/index_management/__jest__/client_integration/home/data_streams_tab.test.ts
Outdated
Show resolved
Hide resolved
...ation/sections/home/data_stream_list/edit_data_retention_modal/edit_data_retention_modal.tsx
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.
Thanks a lot for working on this feature, @sabarasaba!
Left just a couple of nits in the code, but the latest changes LGTM 👍
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
…-ref HEAD~1..HEAD --fix'
6b95d72
to
8189921
Compare
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @sabarasaba |
Fixes: #165136
Summary
This PR adds support for editing data retention for a data stream.
How to test
yarn es snapshot --license=trial
and serverless search solution withyarn start
index management
and click ondata streams
tabCreating a test data stream
Screenshots