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 panic method in crdt text #522

Merged
merged 6 commits into from
Apr 26, 2023
Merged

Remove panic method in crdt text #522

merged 6 commits into from
Apr 26, 2023

Conversation

emplam27
Copy link
Contributor

Remove panic method in crdt text

  • go sdk crdt text has the condition that cause panic
  • remove panic prevent shutdown server when crdt text error occurred

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #501

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

- go sdk crdt text has the condition that cause panic
- remove panic prevent shutdown server when crdt text error occurred
@emplam27 emplam27 self-assigned this Apr 25, 2023
@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #522 (adb262c) into main (969e509) will decrease coverage by 0.78%.
The diff coverage is 37.61%.

❗ Current head adb262c differs from pull request most recent head 350a5cd. Consider uploading reports for the commit 350a5cd to get more accurate results

@@            Coverage Diff             @@
##             main     #522      +/-   ##
==========================================
- Coverage   50.61%   49.84%   -0.78%     
==========================================
  Files          64       64              
  Lines        5751     5860     +109     
==========================================
+ Hits         2911     2921      +10     
- Misses       2471     2534      +63     
- Partials      369      405      +36     
Impacted Files Coverage Δ
pkg/document/crdt/array.go 34.04% <0.00%> (-2.33%) ⬇️
pkg/document/crdt/counter.go 53.08% <0.00%> (ø)
pkg/document/crdt/object.go 21.62% <0.00%> (-1.91%) ⬇️
pkg/document/internal_document.go 21.31% <0.00%> (-0.73%) ⬇️
pkg/document/crdt/root.go 73.33% <25.00%> (-6.67%) ⬇️
pkg/document/crdt/text.go 54.05% <29.16%> (-6.16%) ⬇️
api/converter/from_bytes.go 62.35% <30.00%> (-2.29%) ⬇️
pkg/document/document.go 45.23% <30.00%> (-6.88%) ⬇️
pkg/llrb/llrb.go 69.87% <43.63%> (-10.45%) ⬇️
pkg/document/crdt/rga_tree_split.go 65.05% <45.94%> (-8.45%) ⬇️
... and 2 more

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

@emplam27 Thanks for your contribution.

crdt.Text uses RGATreeSplit and SplayTree. So To remove all panic from crdt.Text, we also need to remove panic in RGATreeSplit and SplayTree.

### pkg/document/crdt/rga_tree_split.go

46:// argument is nil, it would panic at runtime.
49:		panic("RGATreeSplitNodeID cannot be null")
351:		panic("the node of the given id should be found: " + s.StructureAsString())
367:		panic("offset should be less than or equal to length: " + s.StructureAsString())

### pkg/splay/splay.go
206:		panic(fmt.Sprintf(

https://github.com/yorkie-team/yorkie/blob/main/design/data-structure.md#overview

But since SplayTree is also used in Array, I think it would be better to remove it from another PR(e.g. Remove panic in crdt.Array).

@emplam27
Copy link
Contributor Author

@hackerwins
i'm done to remove rga_tree_split's panic.
then, how about element_rht and rga_tree_list's panic? is it ok to remove those panics?

### pkg/document/crdt/element_rht.go
173:		panic("fail to find: " + elem.CreatedAt().Key())

### pkg/document/crdt/rga_tree_list.go
209:		panic("fail to find the given createdAt: " + createdAt.Key())
241:		panic("fail to find the given prevCreatedAt: " + prevCreatedAt.Key())
246:		panic("fail to find the given createdAt: " + createdAt.Key())
261:		panic("fail to find the given prevCreatedAt: " + createdAt.Key())
278:		panic("fail to find the given createdAt: " + elem.CreatedAt().Key())
290:		panic("fail to find the given createdAt: " + createdAt.Key())

@hackerwins hackerwins added the cleanup 🧹 Paying off technical debt label Apr 26, 2023
@hackerwins hackerwins self-requested a review April 26, 2023 13:20
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. 👍

@hackerwins hackerwins merged commit 66c17d6 into main Apr 26, 2023
@hackerwins hackerwins deleted the remove-panic-crdt-text branch April 26, 2023 13:25
@krapie krapie mentioned this pull request Sep 6, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants