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 from crdt.RGATreeList #596

Merged

Conversation

sejongk
Copy link
Contributor

@sejongk sejongk commented Aug 2, 2023

What this PR does / why we need it:
This removes panic in crdt/rga_tree_list that should be avoided in production.

Which issue(s) this PR fixes:
Related to #497

Special notes for your reviewer:
I guess it would be good to add some test cases for errors.
Would you mind giving me some comments about this?
I'm just planning to add tests for a basic success and the errors included in this pr.

Does this PR introduce a user-facing change?:


Additional documentation


Checklist:

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

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #596 (9b3b338) into main (1b557be) will increase coverage by 0.36%.
Report is 4 commits behind head on main.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main     #596      +/-   ##
==========================================
+ Coverage   51.11%   51.47%   +0.36%     
==========================================
  Files          66       66              
  Lines        7008     6962      -46     
==========================================
+ Hits         3582     3584       +2     
+ Misses       2952     2904      -48     
  Partials      474      474              
Files Changed Coverage Δ
api/converter/from_bytes.go 52.42% <0.00%> (-0.75%) ⬇️
pkg/document/crdt/element_rht.go 62.85% <0.00%> (ø)
pkg/document/crdt/object.go 21.62% <ø> (ø)
pkg/document/crdt/array.go 30.35% <33.33%> (-1.72%) ⬇️
pkg/document/crdt/rga_tree_list.go 80.15% <70.00%> (+17.45%) ⬆️

... and 9 files with indirect coverage changes

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.

Overall it looks good. I left a minor comment related to style.
And if possible, it would be nice to add related test codes as well.

pkg/document/crdt/element_rht.go Outdated Show resolved Hide resolved
@@ -214,7 +214,9 @@ func (p *Array) insertAfterInternal(
ticket,
))

p.InsertAfter(prevCreatedAt, value)
if err = p.InsertAfter(prevCreatedAt, value); err != nil {
panic(err)
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me why panic is called?
I think it's safer to handle errors using errors.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, are you going to phase it out?

Copy link
Contributor Author

@sejongk sejongk Aug 2, 2023

Choose a reason for hiding this comment

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

I thought it is okay to call panic in json according to #497 (comment).
It would be great to change the above logic to handle errors if possible.
I guess one of possible errors can be an error that occurs because of a failure of finding a node for the given createdAt.

func (a *RGATreeList) findNextBeforeExecutedAt(
	createdAt *time.Ticket,
	executedAt *time.Ticket,
) (*RGATreeListNode, error) {
	node, ok := a.nodeMapByCreatedAt[createdAt.Key()]
	if !ok {
		return nil, fmt.Errorf("fail to find the given createdAt: %s", createdAt.Key())
	}

	...
}

Would you mind giving me some guides or examples to safely handle errors like the above?

@sejongk sejongk requested a review from hackerwins August 3, 2023 09:01
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.

I left one more minor comment.

pkg/document/crdt/rga_tree_list_test.go Outdated Show resolved Hide resolved
pkg/document/crdt/element_rht.go Outdated Show resolved Hide resolved
@hackerwins hackerwins merged commit c6ed71b into yorkie-team:main Aug 3, 2023
1 check passed
Wu22e pushed a commit to Wu22e/yorkie that referenced this pull request Sep 3, 2023
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.

3 participants