PSD to Spectrum Power units change #30

ejbrever · 2019-11-21T01:07:55Z

@aashaikh Can you please review? This is a continuation of the model change which moved away from the PSD units. Thanks!

googlebot · 2019-11-21T01:07:58Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ejbrever · 2019-11-21T01:20:43Z

@googlebot I signed it!

…

On Wed, Nov 20, 2019 at 5:07 PM googlebot ***@***.***> wrote: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 *Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign.* Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it. ------------------------------ What to do if you already signed the CLA Individual signers - It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. Corporate signers - Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version <https://opensource.google/docs/cla/#troubleshoot>). - The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. - The email used to register you as an authorized contributor must also be attached to your GitHub account <https://github.com/settings/emails>. ℹ️ *Googlers: Go here <https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2Fopenconfig%2Fgnoi%2Fpull%2F30> for more info*. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30?email_source=notifications&email_token=AEUI4LZAZSMSYAEDLYX4343QUXNO5A5CNFSM4JP3OVR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEXRHMQ#issuecomment-556733362>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEUI4L46O6KMBFIZDPBTVSTQUXNO5ANCNFSM4JP3OVRQ> .

googlebot · 2019-11-21T01:20:46Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

aashaikh · 2019-11-26T18:05:19Z

wavelength_router/wavelength_router.proto

@@ -23,7 +23,7 @@ import "github.com/openconfig/gnoi/types/types.proto";
 option (types.gnoi_version) = "0.1.0";


this version will need to be updated -- do we know if there are implementations of this RPC already?

Nope, fortunately, we caught this early on. With some luck, there will be implementations in 2020 :)

Version is now updated. Can you please take another look?

aashaikh · 2019-11-26T18:09:13Z

wavelength_router/wavelength_router.proto

-  rpc AdjustPSD(AdjustPSDRequest) returns (stream AdjustPSDResponse) {}
+  // triggered by a client can be cancelled unless the CancelAdjustSpectrum
+  // RPC is used.
+  rpc AdjustSpectrum(AdjustSpectrumRequest) returns (stream AdjustSpectrumResponse) {}


Do we need to consider add a new rpc rather than just replacing this one? (is there an easier migration path?)

Since there are not yet any implementations, I think this will be the cleanest way long term.

After some additional discussion, I think we should consider adding a new RPC rather than just wholesale replacing. We can mark the existing rpc or message fields as deprecated, and remove it in a subsequent revision.

Sure, the RPCs are marked as deprecated. I can remove in a subsequent revision. Please let me know if this needs anything else.

aashaikh · 2019-12-04T19:15:47Z

wavelength_router/wavelength_router.proto

+  // CancelAdjustSpectrum cancels an in-progress AdjustSpectrum request. If an
+  // adjustment is not being performed for the provided request, then the RPC
+  // should return successfully.
+  rpc CancelAdjustSpectrum(AdjustSpectrumRequest) returns (CancelAdjustSpectrumResponse) {}
 }

 message AdjustPSDRequest {


Do we need to mark these messages deprecated as well ? What is the proto best practice?

I learned something new that a message can be marked deprecated :)

aashaikh · 2019-12-04T19:17:21Z

wavelength_router/wavelength_router.proto

+message AdjustSpectrumRequest {
+  // Absolute path to the /components/component list member in the OpenConfig
+  // model corresponding to the port to perform the adjustment for.
+  // (e.g., /components/component[name=amp-1-1])


wouldn't this be a port on the ROADM (not AMP?) in the case where they are combined, are the ports named for amplifier?

The important part is to indicate directionality to the system as these adjustments are made on a per direction basis. Some vendors use the amp port as a convenient way, but I agree, it logically makes more sense in the example for this to be a waverouter port (it also aligns well with the name of the proto :)

ejbrever

@aashaikh can you please take another look?

ejbrever · 2019-12-05T18:23:19Z

wavelength_router/wavelength_router.proto

+message AdjustSpectrumRequest {
+  // Absolute path to the /components/component list member in the OpenConfig
+  // model corresponding to the port to perform the adjustment for.
+  // (e.g., /components/component[name=amp-1-1])


The important part is to indicate directionality to the system as these adjustments are made on a per direction basis. Some vendors use the amp port as a convenient way, but I agree, it logically makes more sense in the example for this to be a waverouter port (it also aligns well with the name of the proto :)

ejbrever · 2019-12-05T18:38:27Z

wavelength_router/wavelength_router.proto

+  // CancelAdjustSpectrum cancels an in-progress AdjustSpectrum request. If an
+  // adjustment is not being performed for the provided request, then the RPC
+  // should return successfully.
+  rpc CancelAdjustSpectrum(AdjustSpectrumRequest) returns (CancelAdjustSpectrumResponse) {}
 }

 message AdjustPSDRequest {


I learned something new that a message can be marked deprecated :)

aashaikh

LGTM - thanks for the updates!

psd to spectrum power units change

14f05c9

aashaikh reviewed Nov 26, 2019

View reviewed changes

Eric Breverman added 2 commits November 26, 2019 10:26

update version

f50329e

mark old RPCs deprecated instead of deleting

21fdffc

aashaikh reviewed Dec 4, 2019

View reviewed changes

deprecate msgs and fix example

b7e5972

ejbrever commented Dec 5, 2019

View reviewed changes

aashaikh approved these changes Dec 6, 2019

View reviewed changes

ejbrever merged commit b4d663a into master Dec 6, 2019

ejbrever deleted the gnoi-psd branch December 6, 2019 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSD to Spectrum Power units change #30

PSD to Spectrum Power units change #30

ejbrever commented Nov 21, 2019

googlebot commented Nov 21, 2019

ejbrever commented Nov 21, 2019 via email

googlebot commented Nov 21, 2019

aashaikh Nov 26, 2019

ejbrever Nov 26, 2019

ejbrever Nov 26, 2019

aashaikh Nov 26, 2019

ejbrever Nov 26, 2019

aashaikh Dec 3, 2019

ejbrever Dec 3, 2019

aashaikh Dec 4, 2019

ejbrever Dec 5, 2019

aashaikh Dec 4, 2019

ejbrever Dec 5, 2019

ejbrever left a comment

ejbrever Dec 5, 2019

ejbrever Dec 5, 2019

aashaikh left a comment

		@@ -23,7 +23,7 @@ import "github.com/openconfig/gnoi/types/types.proto";
		option (types.gnoi_version) = "0.1.0";

PSD to Spectrum Power units change #30

PSD to Spectrum Power units change #30

Conversation

ejbrever commented Nov 21, 2019

googlebot commented Nov 21, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

ejbrever commented Nov 21, 2019 via email

googlebot commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejbrever left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aashaikh left a comment

Choose a reason for hiding this comment