-
Notifications
You must be signed in to change notification settings - Fork 53
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
add functions to initiate a view context from an existing go context #644
Conversation
|
||
func (cm *manager) InitiateContextFrom(ctx context.Context, view view.View, id view.Identity, contextID string) (view.Context, error) { | ||
if ctx == nil { | ||
cm.contextsSync.Lock() |
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.
I think we don't need the lock here to access cm.ctx.
The field is set when Start is called and Start is called only once at very beginning before any view can be initiated. Wdyt?
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.
updated the PR
@@ -242,21 +242,18 @@ func (cm *manager) InitiateViewWithIdentity(view view.View, id view.Identity, c | |||
} | |||
|
|||
func (cm *manager) InitiateContext(view view.View) (view.Context, error) { | |||
return cm.InitiateContextWithIdentity(view, cm.me()) | |||
return cm.InitiateContextFrom(cm.ctx, view, cm.me(), "") |
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.
We don't need to modify these two methods, right? InitiateContext and InitiateContextWithIdentity
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.
I wanted to keep their signatures as they were to not change the API. That's why I pass the default values to the new InitiateContextFrom function (which has an additional parameter for the context).
} | ||
|
||
func (cm *manager) InitiateContextWithIdentityAndID(view view.View, id view.Identity, contextID string) (view.Context, error) { | ||
// Create the context |
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.
I am iffy about this. Let's first make sure that it works on the CI and on the experiment before merging.
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.
For what it's worth, I ran this in a test application and it works. CI also passes. Do you want to test it anywhere else?
f909fd7
to
120dea5
Compare
test with token sdk succeeds: hyperledger-labs/fabric-token-sdk#762 |
Signed-off-by: Arne Rutjes <[email protected]>
…at node start Signed-off-by: Arne Rutjes <[email protected]>
120dea5
to
55fa65d
Compare
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.
LGTM
for people who don't InitiateView directly but create the view.Context first, this PR creates the opportunity to pass a go context. This is useful for tracing a request through the process.