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

[Native]How about change TransactionListener's method param type from struct to pointer #340

Closed
Me1onRind opened this issue Dec 24, 2019 · 5 comments
Labels
discuss Something undering disscussing
Milestone

Comments

@Me1onRind
Copy link
Contributor

When I try use transaction, I implement the interface, I find the interface method's param is struct

// primitive/message.go
type TransactionListener interface {
    ExecuteLocalTransaction(Message) LocalTransactionState
    CheckLocalTransaction(MessageExt) LocalTransactionState
}

I think passing struct isn't a efficient way. Then, my vim warning me "copy locks"
becuse struct Message and MessageExt anonymous inheritance sync.RWMutex

// primitive/message.go
type Message struct {
    // other codes                                                                                                                                                    
    mutex      sync.RWMutex                                                                                                                                                                  
}
// ...
type MessageExt struct {
    Message
    // other codes
}                                                                                                                                                                  

So I think mabey change TransactionListener to

type TransactionListener interface {
    ExecuteLocalTransaction(*Message) LocalTransactionState
    CheckLocalTransaction(*MessageExt) LocalTransactionState
}

will be better, if it will not case other bug. If it's ok, I can try to change it.

@ShannonDing ShannonDing changed the title How about change TransactionListener's method param type from struct to pointer [Native]How about change TransactionListener's method param type from struct to pointer Dec 31, 2019
@ShannonDing ShannonDing added the discuss Something undering disscussing label Dec 31, 2019
@ShannonDing ShannonDing added this to the 2.0.0-RC1 milestone Dec 31, 2019
@wenfengwang
Copy link
Member

Thank you @Me1onRind , your are right, a pointer is better. could you create a Pull Request for the issue before next Monday(we wiil release rc version in next week) if possible

@Me1onRind
Copy link
Contributor Author

Sorry I am busy these day, and see the message now (>_<), So I can't finished it before Monday now. It not in next rc version may not bad thing, may just change it to pointer may bring some unpredictable hidden bug?

@wenfengwang wenfengwang modified the milestones: 2.0.0-RC1, 2.0.0-RC2 Jan 6, 2020
@wenfengwang
Copy link
Member

@Me1onRind There are no some unpredictable bugs appear in my head, I think the pointer will work well. How do you think of it? @xujianhai666

@Me1onRind
Copy link
Contributor Author

emmm, I just not very familiar with all codes, so afraid bring bug into project^_^. I have created Pull Request.

@wenfengwang
Copy link
Member

close due to #373

@wenfengwang wenfengwang modified the milestones: 2.0.0-RC2, 2.0.0-RC1 Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Something undering disscussing
Projects
None yet
Development

No branches or pull requests

3 participants