-
Notifications
You must be signed in to change notification settings - Fork 24
Initial fault interceptor implementation #551
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
Conversation
Pull Request Test Coverage Report for Build 13530669446Details
💛 - Coveralls |
bormanp
left a comment
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.
You are planning to remove the regex matching, right?
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package fault |
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.
Missing package comment.
Actually, it appears many things are missing godoc comments.
| var recv *faultMessage | ||
| select { // Receive the potential modified req from the fault RPC. | ||
| case recv = <-recvCh: | ||
| case <-time.After(time.Second): |
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.
this should be a constant or variable defined up top.
| } | ||
|
|
||
| i.receiversMu.Lock() | ||
| delete(i.receivers, msgID) |
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.
Shouldn't this be done even if we did not receive a message?
| } | ||
|
|
||
| func (i *Interceptor) sendRecvFault(ch chan *faultMessage, rpcID string, msg any, oErr error) (any, error) { | ||
| msgID := uuid.New().String() |
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.
There is no reason to create this prior to line 72.
| } | ||
|
|
||
| func (i *Interceptor) Unary(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { | ||
| rpcID := uuid.New().String() |
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.
again, not needed to soon.
No description provided.