Subject: Re: [MIB-DOCTORS] Adrian Farrel's Discuss ondraft-ietf-trill-rbridge-mib-04: (withDISCUSS and COMMENT)

From: David Harrington <[email-masked]>
Date: Thu, 02 Feb 2012 18:35:14 -0500

Hi,

There are a couple of points that I think should be mentioned. Hopefully,
I remember all this correctly and don't generate FUD.

1) the IETF Bridge MIB modules are "frozen in time", and the IETF
standards based on these modules is still valid. IEEE changed the IETF
Bridge MIB modules to support provider-based bridging. They modified the
indexing to include an instance identifier, where the IETF modules lack
such an index field. Since they modified indexing, they needed to redefine
the OID registrations. They chose to relocate the root of the module to an
IEEE OID tree. So there are two valid standard trees for managing bridges
- the IETF standard modules and the IEEE standard modules.

2) The IETF standard has been implemented widely and is especially useful
for enterprises (where only one instance of the Bridge MIB is needed) -
that's why we kept it valid; the IEEE standard is useful for service
providers, where support for multiple bridging instances is needed. I
think the IEEE modules may be used with a single instance, but I don't
know how the IETF and IEEE standards compare otherwise.

3) I am a bit concerned about building upon the "frozen in time" IETF
modules. It is probably preferable to build upon the IEEE modules so we
only have one tree growing. I don't know whether TRILL is really focused
on enterprise functionality and would have no use for multiple rbridge
support - but if they can implement single rbridge support based on the
IEEE modules, that should probably be done in case somebody decides to
support multiple rbridge instances.

4) Adrian points out some defaults that are defined in text but not
written in DEFVAL clauses. Most operators use tools that automate SNMP
queries and interpretation, so making the MIB modules machine-readable is
important. In addition there are tools that check MIB module
implementations for compliance to the standards. These tools can check
defaults if they are specified in DEFVAL clauses; these tools would have
trouble reading the English description of a default. Therefore it is
important to use DEFVALs when defining defaults. That may not be obvious
to an agent implementer, but it is important for interoperability between
agent and manager implementations.
 
-- 
--
David Harrington
Director, Transport Area
Internet Engineering Task Force (IETF)
[email-masked]
+1-603-828-1401

On 1/26/12 10:43 AM, "Romascanu, Dan (Dan)" <[email-masked]> wrote:

>This is great, please go ahead, thanks Joan!
>
>Regards,
>
>Dan
>
>
>
>> -----Original Message-----
>> From: Joan Cucchiara [mailto:[email-masked]]
>> Sent: Thursday, January 26, 2012 5:18 PM
>> To: Romascanu, Dan (Dan)
>> Subject: Re: [MIB-DOCTORS] Adrian Farrel's Discuss ondraft-ietf-trill-
>> rbridge-mib-04: (withDISCUSS and COMMENT)
>> 
>> Dan,
>> 
>> If you haven't had any volunteers yet, then I'm happy to do a review,
>> although this
>> isn't my area, but I can do it from a MIB perspective.
>> 
>> Let me know.  (Oh timeframe would be about Feb 7th or so, possibly
>> earlier).
>> 
>> Thanks,
>>   -Joan
>> 
>> ----- Original Message -----
>> From: "Romascanu, Dan (Dan)" <[email-masked]>
>> To: "Ralph Droms" <[email-masked]>; <[email-masked]>
>> Cc: "Adrian Farrel" <[email-masked]>
>> Sent: Tuesday, January 24, 2012 8:17 AM
>> Subject: Re: [MIB-DOCTORS] Adrian Farrel's Discuss
>> ondraft-ietf-trill-rbridge-mib-04: (withDISCUSS and COMMENT)
>> 
>> 
>> > MIB Doctors,
>> >
>> > Please step ahead to help with reviewing this document. If I
>> understand
>> > well its progress depends now on its MIB review! Note that Bert and
>> me
>> > have already reviewed previous versions, and that O have already
>> entered
>> > a DISCUSS on this version related to rbridgeBaseNicknameTable and
>> > rbridgeEsadiTable including read-create objects but now RowStatus
>> > objects. However, a complete MIB Doctor review is necessary.
>> >
>> > Thanks and Regards,
>> >
>> > Dan
>> >
>> >
>> >
>> >
>> >> -----Original Message-----
>> >> From: Ralph Droms [mailto:[email-masked]]
>> >> Sent: Tuesday, January 24, 2012 3:03 PM
>> >> To: [email-masked]
>> >> Cc: Adrian Farrel; Romascanu, Dan (Dan)
>> >> Subject: Re: Adrian Farrel's Discuss on draft-ietf-trill-rbridge-
>> mib-
>> >> 04: (withDISCUSS and COMMENT)
>> >>
>> >> MIB Doctors - I don't think I've seen a reply to Dan's request for
>a
>> >> formal MIB Doctor review of draft-ietf-trill-rbridge-mib-04.  The
>> lack
>> >> of a MIB Doctor review is blocking progress on this document, so I
>> >> would greatly appreciate one of you volunteering to review the
>> >> document.
>> >>
>> >> - Ralph
>> >>
>> >> On Dec 1, 2011, at 10:08 AM 12/1/11, Romascanu, Dan (Dan) wrote:
>> >>
>> >> > MIB Doctors,
>> >> >
>> >> > Please see the DISCUSS entered by Adrian. Both Bert and me
>> provided
>> >> comments on the earlier versions of the MIB module but did not have
>> >> time for a full MIB review. I sent a formal call for volunteers to
>> > this
>> >> list when the document was sent to IETF LC (on 10/30), but this one
>> > was
>> >> not answered. Sure the IETF meeting interfered, but right now the
>> lack
>> >> of a MIB doctors review blocks the document, and the there seem to
>> be
>> >> some problems (or at least questions) raised by Adrian.
>> >> >
>> >> > Please jump in to help.
>> >> >
>> >> > Thanks and Regards,
>> >> >
>> >> > Dan
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > -----Original Message-----
>> >> > From: [email-masked] [mailto:[email-masked]] On
>> Behalf
>> >> Of Adrian Farrel
>> >> > Sent: Thursday, December 01, 2011 4:59 PM
>> >> > To: The IESG
>> >> > Cc: [email-masked]; trill-
>> >> [email-masked]
>> >> > Subject: Adrian Farrel's Discuss on draft-ietf-trill-rbridge-mib-
>> 04:
>> >> (withDISCUSS and COMMENT)
>> >> >
>> >> > Adrian Farrel has entered the following ballot position for
>> >> > draft-ietf-trill-rbridge-mib-04: Discuss
>> >> >
>> >> > When responding, please keep the subject line intact and reply to
>> > all
>> >> > email addresses included in the To and CC lines. (Feel free to
>cut
>> >> this
>> >> > introductory paragraph, however.)
>> >> >
>> >> > Please refer to http://www.ietf.org/iesg/statement/discuss-
>> >> criteria.html
>> >> > for more information about IESG DISCUSS and COMMENT positions.
>> >> >
>> >> >
>> >> >
>> >> >
>> >
>---------------------------------------------------------------------
>> >> -
>> >> > DISCUSS:
>> >> >
>> >
>---------------------------------------------------------------------
>> >> -
>> >> >
>> >> > Has a MIB doctor review been done yet? It seems like one is still
>> >> > needed. I am not a MIB expert, but I think I see a number of
>> issues.
>> >> >
>> >> > ---
>> >> >
>> >> > I'm afraid Sean's Comment needs to be captured in a Discuss.
>> >> >
>> >> > Please resolve the downref to RFC 4663 either by moving it to
>> >> > Informative (which seems reasonable) or by reissuing the last
>> call.
>> >> >
>> >> > ---
>> >> >
>> >> > I would like help from the OPS ADs (and MIB Doctors) on this
>> point:
>> >> >
>> >> > It seems to me that section 6.7 needs a new conformance statement
>> to
>> >> > be applied to ISIS-MIB. The text in the sectionappears to be in
>> two
>> >> > parts:
>> >> >
>> >> > - Implementation guidance (what value to return on a GET from a
>> > Trill
>> >> >  implementation)
>> >> >
>> >> > - Implementation requirements (which tables/objects to implement
>> or
>> >> not
>> >> >  (conformance statement).
>> >> >
>> >> > ---
>> >> >
>> >> >   RbridgeNickname ::= TEXTUAL-CONVENTION
>> >> >       DISPLAY-HINT "d"
>> >> >       STATUS current
>> >> >       DESCRIPTION
>> >> >           "The 16-bit identifier used in TRILL as an
>> >> >           abbreviation for the RBridge's 48-bit IS-IS System ID.
>> >> >           The value 0 means a nickname is not specified, the
>> values
>> >> >           0xffco through 0xfffe are reserved for future
>> allocation,
>> >> >           and the value 0xffff is permanently reserved."
>> >> >   SYNTAX Unsigned32 (0..65471)
>> >> >
>> >> > As specified, if the reserved values are ever allocated for any
>> >> reason
>> >> > you will have to respin the MIB module. You probably don't want
>to
>> >> have
>> >> > to do that, so why not allow 0..65534? (By the way, if 0xffff is
>> >> outside
>> >> > the available range, it is by definition permanetly reserved, so
>> >> maybe
>> >> > you don't need to say anytng about it.)
>> >> >
>> >> > ---
>> >> >
>> >> > There seems to be some confusion about defaults.
>> >> >
>> >> > There are a number of cases where the DESCRIPTION clause states a
>> >> > specific value for the default of a read-write or a read-create
>> >> object.
>> >> > For example:
>> >> >
>> >> >   rbridgeBaseForwardDelay OBJECT-TYPE
>> >> >       SYNTAX      Unsigned32
>> >> >       UNITS       "seconds"
>> >> >       MAX-ACCESS  read-write
>> >> >       STATUS      current
>> >> >       DESCRIPTION
>> >> >           "Modified aging time for address entries after an
>> > appointed
>> >> >           forwarder change. The default value is 15.
>> >> >
>> >> >           The value of this object MUST be retained across
>> >> >           reinitializations of the management system."
>> >> >       REFERENCE
>> >> >            "RBridge section 4.8.2"
>> >> >      ::= { rbridgeBase 3 }
>> >> >
>> >> > This appears to be missing a DEFVAL clause unless you mean
>> something
>> >> > different by "default". If you actually mean that the default in
>> an
>> >> > implementation of the protocol is 15, then it is not appropriate
>> to
>> >> > mention it here.
>> >> >
>> >> > In other objects (rbridgeBaseUniMultipathEnable,
>> >> > rbridgeBaseMultiMultipathEnable) you have "The default is
>> >> > implementation-specific." If this applies to the implementation
>of
>> >> the
>> >> > management agent then you might guide the implementer. If it
>> applies
>> >> to
>> >> > the implementation of the protocol, then it doesn't really belong
>> >> here.
>> >> >
>> >> > I suggest you look at each DESCRIPTION clause where there is a
>> >> default
>> >> > described and work out whether you should:
>> >> > - add a DEFVAL clause
>> >> > - reomove the txt
>> >> > - extend the text to give additional advice to the implementer of
>> > the
>> >> >  management agent.
>> >> >
>> >> > (By the way, sometimes you have included the DEFVAL just fine.)
>> >> >
>> >> > ---
>> >> >
>> >> >   rbridgeBasePortIfIndex OBJECT-TYPE
>> >> >       SYNTAX      InterfaceIndex
>> >> >
>> >> > Should this be InterfaceIndexOrZero? What would happen if the
>> entry
>> >> was
>> >> > deleted from IF-MIB?
>> >> >
>> >> > ---
>> >> >
>> >> > I expected to see some mention of rbridgeSnoopingAddrType in the
>> >> > conformance statement to limit its applicability to only a few of
>> > the
>> >> > address types.
>> >> >
>> >> > ---
>> >> >
>> >> >   rbridgeDtreeActiveTrees OBJECT-TYPE
>> >> >       SYNTAX      Unsigned32
>> >> >
>> >> > Shouldn't this more properly be a Gauge32?
>> >> >
>> >> >
>> >> >
>> >
>---------------------------------------------------------------------
>> >> -
>> >> > COMMENT:
>> >> >
>> >
>---------------------------------------------------------------------
>> >> -
>> >> >
>> >> > You should mainly be saying "MIB module" where you say "MIB"
>> >> >
>> >> > ---
>> >> >
>> >> > Some of the IMPORTS clause entries usefully point to the RFCs
>> > housing
>> >> > the imported TCs. It would be great if you could include the
>> >> references
>> >> > for all the others.
>> >> >
>> >> > ---
>> >> >
>> >> > I appreciate the many REFERENCE clauses. Could add one to
>> >> > RbridgeNickname.
>> >> >
>> >> > ---
>> >> >
>> >> >   -- Implementation of the rbridgeBase subtree is mandatory for
>> all
>> >> >   -- RBridges.
>> >> >
>> >> > Does this mean
>> >> > ... that are conformant to this MIB module
>> >> > Or are you making a wider statement about Rbridges?
>> >> >
>> >> > ---
>> >> >
>> >> > The DESCRIPTION clause of rbridgeSnoopingPortAddr should indicate
>> > the
>> >> > (obvious) fact that the object is interpretted in the context of
>> >> > rbridgeSnoopingPortAddrType.
>> >> >
>> >> > ---
>> >> >
>> >> >   rbridgeDtreeActiveTrees OBJECT-TYPE
>> >> >       SYNTAX      Unsigned32
>> >> >       MAX-ACCESS  read-only
>> >> >       STATUS      current
>> >> >       DESCRIPTION
>> >> >           "The total number of trees being computed by all
>> Rbridges
>> >> >           campus."
>> >> >
>> >> > Missing some text in the DESCRIPTION clause?
>> >> >
>> >> >
>> >
>> > _______________________________________________
>> > MIB-DOCTORS mailing list
>> > [email-masked]
>> > https://www.ietf.org/mailman/listinfo/mib-doctors
>
>_______________________________________________
>MIB-DOCTORS mailing list
>[email-masked]
>https://www.ietf.org/mailman/listinfo/mib-doctors

---

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Fri, 03 Feb 2012 10:35:41 +0100

I agree with the below from David.

iirc, then I have brought this up several months ago when I did
a very intial and quick skim of the MIB module.

I am not the one to decide what they need to base their MIB module
on. But some good explanation as to why they choose the IETF
"frozen in time" one over the IEEE one would be a good piece
of text to add to the document, maybe even in the DESCRIPTION
clause of the MODULE-IDENTITY.

Bert.

---

From: "Romascanu, Dan (Dan)" <[email-masked]>
Date: Fri, 3 Feb 2012 10:48:58 +0100

I agree with Bert and David on this. 

Regards,

Dan

> From: Bert Wijnen (IETF) [mailto:[email-masked]]
> Sent: Friday, February 03, 2012 11:36 AM
> To: David Harrington
> Cc: Romascanu, Dan (Dan); Joan Cucchiara; Donald Eastlake; MIB
Doctors;
> Anil Rijhsinghani; Ralph Droms; Adrian Farrel

---

From: "Bert Wijnen (IETF)" <[email-masked]>
Date: Fri, 03 Feb 2012 13:20:48 +0100

Thanks Anil for the update.
I have not read the more recent versions.
Hope to find some time for that later.

Importing a few TCs should be fine.
iirc (but I would have to check) then then the IEEE versions
also import soem TCs from IETF MIB modules, but I would also
need to re-check that.

Bert

On 2/3/12 1:06 PM, Anil Rijhsinghani wrote:
> This is all in the descriptive part of the MIB. As far as actual syntax, all that has been taken from the IETF MIBs is a couple of TEXTUAL-CONVENTIONs. If this is of concern, let me know and we can try to replace them with standalone typedefs.

---

From: "Romascanu, Dan (Dan)" <[email-masked]>
Date: Fri, 3 Feb 2012 17:13:25 +0100

Hi Joan,

I hope that all is fine with you and that you are making progress with
your review. Please make sure that you are looking at the latest version
submitted on 1/30. FYI, David Harrington entered a DISCUSS that includes
some relevant comments that may also help - see
https://datatracker.ietf.org/doc/draft-ietf-trill-rbridge-mib/ballot/

Thanks and Regards,

Dan

> From: [email-masked] [mailto:mib-doctors-
> [email-masked]] On Behalf Of Romascanu, Dan (Dan)
> Sent: Thursday, January 26, 2012 5:43 PM
> To: Joan Cucchiara
> Cc: [email-masked]; Anil Rijhsinghani; Ralph Droms; Adrian
Farrel
> Subject: Re: [MIB-DOCTORS] Adrian Farrel's Discussondraft-ietf-trill-

---

From: "Joan Cucchiara" <[email-masked]>
Date: Sat, 4 Feb 2012 08:50:14 -0500

Will do!

----- Original Message ----- 
From: "Romascanu, Dan (Dan)" <[email-masked]>
To: "Romascanu, Dan (Dan)" <[email-masked]>; "Joan Cucchiara" 
<[email-masked]>
Cc: <[email-masked]>; "Anil Rijhsinghani" <[email-masked]>; "Ralph 
Droms" <[email-masked]>; "Adrian Farrel" <[email-masked]>; 
<[email-masked]>
Sent: Friday, February 03, 2012 11:13 AM
Subject: RE: [MIB-DOCTORS] Adrian Farrel's 
Discussondraft-ietf-trill-rbridge-mib-04: (withDISCUSS and COMMENT)

Hi Joan,

Thanks and Regards,

Dan

Farrel

---

From: "Joan Cucchiara" <[email-masked]>
Date: Wed, 8 Feb 2012 12:37:33 -0500

MIB Dr. Review:  draft-ietf-trill-rbridge-mib-06.txt
====================================================

Hello Anil,

The MIB is very well organized and obviously much work has
gone into the draft. This has made it nice to review.
Thank you for that!

The review begins with output from the MIB compilers, followed by
some comments.  Where appropriate, I have marked comments as SUGGEST (only
my suggestion and entirely upto you).  NIT which is usually a typo or 
something
very minor.

In general, there are still some places where the Default Values are only 
specified in
the DESCRIPTION and they should be  put into a DEFVAL clause.  Also, there 
probably
should be SIZE restrictions on some values.  The compiler output suggests 
some
of these places.

MIB compiler output is followed by a couple of General Comments, and then 
specific
comments.  The MIB is quite long so there are many comments but most of the 
comments
are quite similar.

Thanks,
  -Joan

MIB COMPILER OUTPUT:
======================
smicng output:
W: f(RBRIDGE-MIB.my), (330,4) Sequence "RBridgeBasePortEntry" and Row 
"rbridgeBasePortEntry" should have related names
W: f(RBRIDGE-MIB.my), (912,20) Item "rbridgeMultiFibPorts" should have SIZE 
specified
W: f(RBRIDGE-MIB.my), (1429,4) Row "rbridgeSnoopingAddrEntry" has indexing 
that may create variables with more than 128 sub-ids
W: f(RBRIDGE-MIB.my), (1475,20) Item "rbridgeSnoopingAddrPorts" should have 
SIZE specified
W: f(RBRIDGE-MIB.my), (1035,18) Row "rbridgeVlanPortEntry" does not have a 
consistent indexing scheme - cannot specify an index item from additional 
"base row" rbridgeVlanEntry, since can have only one "base row" which is 
rbridgeBasePortEntry

smilint output:
================
mibs/RBRIDGE-MIB.my:1429: [5] {index-exceeds-too-large} warning: index of 
row `rbridgeSnoopingAddrEntry' can exceed OID size limit by 142 
subidentifier(s)

Additional descriptions of some error/warning messages:

Error:       index-element-no-size (level 3)
Message:     index element `%s' of row `%s' must have a size restriction
Description: Object identifiers are restricted in size to have at most 128
             sub-identifiers. This implies that index elements used to form
             instance identifiers must have a size contraint which ensures
             that the 128 sub-identifier constraint is kept intact for any
             possible combination of the index elements' values (RFC 2578,
             Section 3.5).

* Given the above, I would like to confirm that rbridgeSnoopingAddrEntry 
needs to
exceed the 128 sub-id limit.

Specifically, why do rbridgeSnoopingAddrType, rbridgeSnoopingAddr  not 
specify
a specific type of address (such as IPv4 and IPv6)?   I think they probably 
could
but wanted to ask?

6.7
NIT:  Header
s/IS-IS MIB/ISIS-MIB

Other places in this doc also should also change to ISIS-MIB.

Comments about MIB Module
============================

GENERAL COMMENTS
------------------

*) General Comment:  Please check the REFERENCE clauses
so that they consistantly refer to "RFC 6325", and not
"RBridge" or "RBridge clause".

*) While DEFVALs have been added in many place, they have not
been added in all places.  Please check this.

*) TruthValue - This is used extensively throughout the MIB Module.
While that is probably okay, want to make sure that this isn't limiting,
such that maybe an object would benefit by having

INTEGER {
   unknown(1),
   enabled(2)
   disabled(3) }

I'm not asking for a change here, but would ask that you take a look
at the TruthValue objects to make sure this is really want you want,
now and in the future.  NOTE:  If these objects represent a single bit
in the specification like multi-destination, then true/false is fine.
However, not sure this is the case with all of the TruthValue objects.

*) RbridgeNickName
Syntax Unsigned32(0|1..65471)

Separating "0" because this is a special case.  While, the range is the same 
as what you
have, this is a readability issue.

*) rbridgeBaseTrillVersion
Isn't this a 2 bit field?  Why no SIZE range?
Also, think section 3.2 is a better reference.
DEFVAL {0}

*) rbridgeBaseNumPorts, should this be a Guage32?

I don't understand the UNITS clause on this?  It is supposed
to help add info and it doesn't IMHO.

*) rbridgeBaseForwardDelay

Think Reference is supposed to be Section 4.8.3?
Also, isn't this supposed to be 4..30 with DEFVAL of 15 seconds?

*) rbridgeBaseUniMultipathEnable, rbridgeBaseMultiMultipathEnable

These objects should be optional, however, they are required by
the Conformance Statement.  Please Explain.

DESCRIPTION
Please state what the value of True(1) and False(2) mean,
as in "When the value of this object is True(1), multipath
is enabled."

REFERENCE
Should specify the RFC (RFC6325, Appendix C)

DEFVAL
These should have a DEFVAL.  (Probably False(2) since these
are optional?)

* rbridgeBaseNicknameNumber

Discuss:  I don't understand the relationship between this object
and the rbridgeBaseNicknameTable.  Is this the MAX number of
entries in the Table?  Or does this value represent the current
entries in the Table?  If it is Max, please say Max somewhere in
the object's name.  Also, I think the value is supposed to be
1..256, with DEFVAL of 1.

DESCRIPTION clause still gives a default value.
Please remove from DESCRIPTION and give a proper DEFVAL { 1 }

Should also have a REFERENCE.

SUGGEST:  Please consider moving this scalar to be before the table (with 
appropriate OID
changes).

* rbridgeBaseAcceptEncapNonadj
DESCRIPTION clause still gives a default value.
Please remove from DESCRIPTION and give a proper DEFVAL { false }

Discuss: This value is supposed to be per port, right?   Is this scalar
per port?

rbridgeBasePortTable

* rbridgeBasePort  (should probably be Unsigned32)

* rbridgeBasePortIfIndex
Which IfIndex is this referring to?  In other words,
what is the corresponding ifType?

*rbridgeBasePortStpRootChanges

Which Discontinuity Object is being referred to here?
Is this is the corresponding ifCounterDiscontinuityTime object
then please state that in the DESCRIPTION.

*rbridgeConfidenceNative, rbridgeConfidenceDecap
Need DEFVALs

Do these scalars refer to the entire Rbridge or is this
per port?

*rbridgeUniFdbPort, should probably an Unsigned32.
DEFVAL { 0 }

*rbridgeUniFdbPort should probably be Unsigned32.
Are the ports in this object related to the ports
in the rbridgeBasePortTable ?   Please clarify.

*rbridgeUniFdbNick
Could you consistently use Nickname as was done
in prior objects?  Does this value appear in the
rbridgeNickTable?  Please clarify.

* rbridgeUniFdbConfidence
I think this could also be 255 if configured by management?
(My question is because the following object (status) includes
management.
Also, should this have a DEFVAL of 0x20 as in 4.8.2 ?

* rbridgeUniFdbStatus

REFERENCE ?

* rbridgeUniFibNickname
Is this the same Nickname that has an entry in rbridgeNickTable?
If so, please clarify that in the DESCRIPTION.

*rbridgeUniFibPort
Unsigned32 ?
Are the ports in this object related to the ports
in the rbridgeBasePortTable ?   Please clarify.

*rbridgeMultiFibPorts
Does this PortList correspond to entries in the rbridgeBasePortTable?
Please explain.

*rbridgeVlanForwarderLost
Counters should end in an "s" to indicate possibly more than one.
Please rename to rbridgeVlanForwarderLost to rbridgeVlanForwarderLostChanges

DESCRIPTION -- So counters are usually retrieved along with their 
corresponding
Discontinuity Objects.  If there is a change in the Discontinuity Object, 
that
indicates the counter suffered a discontinuity.  Please specify what the
Discontinuity object is for this counter.

REFERENCE -- think this is 4.8.3

* rbridgeVlanDisableLearning
Please clarify as to what True(1) and False(2) mean here, e.g.:
To disable learning....the value of this object should be false (2).
Otherwise, true(1).

* rbridgeVlanSnooping
Should there be a separate ipv6 value?

* rbridgeVlanPortTable

I'm not clear on why this is indexed the way it is.  Why wouldn't you
want Port first?

*rbridgePortCounterTable
As mentioned prior, counters should end in "s".

* rbridgeEsadiEnable
Please clearly state the values of True/False for this
object.

*rbridgeEsadiConfidence
REFERENCE  and Section 4.8.2

Why is 255 excluded as a value?

*rbridgeEsadiDrbHoldingTime

DEFVAL ?

*rbridgeSnoopingPortAddrType and rbridgeSnoopingPortAddr

Should these be restricted to IPv4 and IPv6 in the conformance
statements?

* rbridgeSnoopingAddrType and rbridgeSnoopingAddr
Same question as above.

*rbridgeDtreePriority

Unsigned32 ?
DEFVAL needed.

* rbridgeDtreeActiveTrees
Not sure if this is supposed to be a Counter32 ?
If so, then is there a Discontinuity associated with it?

* rbridgeDtreeMaxTrees

is the DEFVAL [ 1 } ??

*rbridgeDtreeNumber

Where did these value come from?
       SYNTAX      Unsigned32 (0..65535)

*rbridgeDtreeNick
Please use Nickname to be consistant with rest of the MIB module.

*rbridgeTrillSz
Why is this read-only?

* rbridgeTrillNbrMtu
Is this supposed to be "to this neighbor" or "for this neighbor"?

*rbridgeBaseNewDrb

Please don't use the word "trap".  These are Notifications.

I don't understand why this notification is optional.  Is there
a concern that too many of these would be sent, or is it just
not a very important event to be notified about?
Please explain.

*rbridgeBaseTopologyChange

Similar comment as above. My concern is that maybe a throttling
mechanism may be better than to just say this optional.

Compliance/Conformance Statements

As mentioned in above comments, limiting values for InetAddrType
and InetAddr and RowStatus objects may be appropriate.

Additionally, many MIBs have a "Full" conformance and a 'ReadOnly"
conformance statement.  This MIB only has a Full Conformance
(i.e. vendors have to support read-write and read-create objects).
That is fine, if this is what the Working Group wants.  Please
be sure this is what is wanted.