USI - #496
USI#496
Conversation
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
==========================================
- Coverage 79.89% 79.86% -0.03%
==========================================
Files 21 21
Lines 3621 3621
==========================================
- Hits 2893 2892 -1
- Misses 728 729 +1
Continue to review full report at Codecov.
|
grafnu
left a comment
grafnu
left a comment
There was a problem hiding this comment.
Also waiting for stickler/checkstyle, although that's more just formality.
|
|
||
| COPY usi/ usi/ | ||
|
|
||
| RUN cd usi && mvn clean compile assembly:single |
There was a problem hiding this comment.
generally we prefer gradle over mvn, but I know this is just copying what the previous module did. So, it's fine for this, but mostly just FYI for anything new please use gradle not mvn.
There was a problem hiding this comment.
I prefer gradle as well actually.
| @@ -0,0 +1,189 @@ | |||
| package switchtest; | |||
There was a problem hiding this comment.
I don't think the package is right anymore, since this isn't a test -- package should be something like "daq.usi" (note we're avoiding com.google since technically DAQ is part of faucetsdn)
| package switchtest; | ||
|
|
||
| /* | ||
| * Licensed to the Google under one or more contributor license agreements. |
There was a problem hiding this comment.
"to the Google"? :-) -- I know not you, just something I noticed!
| @@ -0,0 +1,92 @@ | |||
| package switchtest; | |||
|
|
|||
| import grpc.*; | |||
There was a problem hiding this comment.
I don't think we ever converged on the "why grpc" question in the design proposal -- it seems to have been deleted without resolution. Also the design proposal was never finalized (still indicated as draft). I want to understand "why grpc" here before proceeding... it's a significant bit of complexity for something that could really be simple. (If we already had a conversation about it please remind me.) I'm not saying it's wrong, I just want to make sure we're properly reviewing/discussing things before executing on them (it's a procedural thing).
There was a problem hiding this comment.
I remember seeing a comment related to grpc in my email but I couldn't find the comment when I went to the design doc. I don't know any good alternatives for this purpose?
|
It's in the document comment history. Also should go through the process
of finalizing the doc (from draft --> (review) --> approved). We can close
it out there. I'm mostly looking for a high-level description (in the
doc), about what the RPC is -- IIRC, it never really says what the contents
of the RPC are.
…On Tue, Jun 23, 2020 at 9:45 AM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In usi/src/main/java/switchtest/USIImpl.java
<#496 (comment)>:
> @@ -0,0 +1,92 @@
+package switchtest;
+
+import grpc.*;
I remember seeing a comment related to grpc in my email but I couldn't
find the comment when I went to the design doc. I don't know any good
alternatives for this purpose?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#496 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPDYVJELWWK73AEZWBULRYDL27ANCNFSM4OFBQZBQ>
.
|
| /** | ||
| * Abstract Switch controller. Override this class for switch specific implementation | ||
| * @param remoteIpAddress switch ip address | ||
| * @param telnetPort switch telnet port |
|
PTAL |
|
Looks good from what I can see... want to make sure the Design Proposal piece is finalized first, and then we can close this out. |
grafnu
left a comment
grafnu
left a comment
There was a problem hiding this comment.
Doc is close enough, and code looks good, thx!
2 participants
No description provided.