Details

    1. hbase.proto
      4 kB
      Devaraj Das
    2. HMasterProtocol.proto
      6 kB
      Devaraj Das
    3. HMasterInterfaceMappingv0.pdf
      5 kB
      Gregory Chanan

      Issue Links

        Activity

        Hide
        Devaraj Das added a comment -

        Attaching the (incomplete) proto definitions for HMasterInterface that I did sometime back. Greg, you might find them useful.

        Show
        Devaraj Das added a comment - Attaching the (incomplete) proto definitions for HMasterInterface that I did sometime back. Greg, you might find them useful.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4283/
        -----------------------------------------------------------

        Review request for hbase.

        Summary
        -------

        First draft of the protobufs specification for HMasterInterface.
        This is relatively close to a one-to-one mapping with the existing interface. A pdf listing the existing-to-protobufs mapping is available on the JIRA: https://issues.apache.org/jira/browse/HBASE-5445

        Thanks to Devaraj who provided some initial work he had done on this.

        This addresses bug HBASE-5445.
        https://issues.apache.org/jira/browse/HBASE-5445

        Diffs


        src/main/proto/HMasterProtocol.proto PRE-CREATION
        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4283/diff

        Testing
        -------

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4283/ ----------------------------------------------------------- Review request for hbase. Summary ------- First draft of the protobufs specification for HMasterInterface. This is relatively close to a one-to-one mapping with the existing interface. A pdf listing the existing-to-protobufs mapping is available on the JIRA: https://issues.apache.org/jira/browse/HBASE-5445 Thanks to Devaraj who provided some initial work he had done on this. This addresses bug HBASE-5445 . https://issues.apache.org/jira/browse/HBASE-5445 Diffs src/main/proto/HMasterProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4283/diff Testing ------- Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4283/#review5868
        -----------------------------------------------------------

        Looks good. Just a few comments below.

        src/main/proto/HMasterProtocol.proto
        <https://reviews.apache.org/r/4283/#comment12790>

        Reference your nice PDF?

        BTW, in your PDF, you have one method that is 'not used' so you don't include it. Do we need to deprecate it in 0.94 so its safe to remove by the time 0.96 comes around? If so, mind filing an issue or just me know and I can take care of it.

        Ditto for loadBalancerSwitchIs(LoadBalancerSwitchIsRequest). This method is just broke from its name and to how it works. Can we deprecate it in 0.94? Add a method that makes sense in 0.94, one that makes sense that you can redo in pb?

        Otherwise, the pdf looks good.

        src/main/proto/HMasterProtocol.proto
        <https://reviews.apache.org/r/4283/#comment12791>

        Did Benoit comment that protobuf is too long, just do pb (Ask Jimmy? He'd remember)

        src/main/proto/HMasterProtocol.proto
        <https://reviews.apache.org/r/4283/#comment12792>

        Is this a good name for this class? Drop the H? Can we change it?

        src/main/proto/HMasterProtocol.proto
        <https://reviews.apache.org/r/4283/#comment12793>

        These are in the master protocol but are they used elsewhere? If so, should they be in here? Maybe they are not and its just me confused.

        src/main/proto/HMasterProtocol.proto
        <https://reviews.apache.org/r/4283/#comment12794>

        Is this class deprecated (HServerInfo?)

        src/main/proto/HMasterProtocol.proto
        <https://reviews.apache.org/r/4283/#comment12795>

        Yeah, this method is broke as said above. We should try do patch up work in 0.94 so you can come in all clean and shiny in 0.96.

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12796>

        The package in first class is protobuf.generated. Here its generated.protobuf. Did you mean that?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12797>

        What is one of these? A bit of a comment?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12798>

        ditto. Whats this map too?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12799>

        We used to 'guess' this from the bytes passed. You are changing that?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12800>

        And we'd generate the encoded name from these articles?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12801>

        uint32 seems wide for number of stores

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12802>

        ditto

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12803>

        This is ServerName? Why call it ServerSpecifier? It should be a different name?

        src/main/proto/hbase.proto
        <https://reviews.apache.org/r/4283/#comment12804>

        Should ColumnMetadata and TableMetadata have common type?

        • Michael

        On 2012-03-10 02:09:45, Gregory Chanan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4283/

        -----------------------------------------------------------

        (Updated 2012-03-10 02:09:45)

        Review request for hbase.

        Summary

        -------

        First draft of the protobufs specification for HMasterInterface.

        This is relatively close to a one-to-one mapping with the existing interface. A pdf listing the existing-to-protobufs mapping is available on the JIRA: https://issues.apache.org/jira/browse/HBASE-5445

        Thanks to Devaraj who provided some initial work he had done on this.

        This addresses bug HBASE-5445.

        https://issues.apache.org/jira/browse/HBASE-5445

        Diffs

        -----

        src/main/proto/HMasterProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4283/diff

        Testing

        -------

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4283/#review5868 ----------------------------------------------------------- Looks good. Just a few comments below. src/main/proto/HMasterProtocol.proto < https://reviews.apache.org/r/4283/#comment12790 > Reference your nice PDF? BTW, in your PDF, you have one method that is 'not used' so you don't include it. Do we need to deprecate it in 0.94 so its safe to remove by the time 0.96 comes around? If so, mind filing an issue or just me know and I can take care of it. Ditto for loadBalancerSwitchIs(LoadBalancerSwitchIsRequest). This method is just broke from its name and to how it works. Can we deprecate it in 0.94? Add a method that makes sense in 0.94, one that makes sense that you can redo in pb? Otherwise, the pdf looks good. src/main/proto/HMasterProtocol.proto < https://reviews.apache.org/r/4283/#comment12791 > Did Benoit comment that protobuf is too long, just do pb (Ask Jimmy? He'd remember) src/main/proto/HMasterProtocol.proto < https://reviews.apache.org/r/4283/#comment12792 > Is this a good name for this class? Drop the H? Can we change it? src/main/proto/HMasterProtocol.proto < https://reviews.apache.org/r/4283/#comment12793 > These are in the master protocol but are they used elsewhere? If so, should they be in here? Maybe they are not and its just me confused. src/main/proto/HMasterProtocol.proto < https://reviews.apache.org/r/4283/#comment12794 > Is this class deprecated (HServerInfo?) src/main/proto/HMasterProtocol.proto < https://reviews.apache.org/r/4283/#comment12795 > Yeah, this method is broke as said above. We should try do patch up work in 0.94 so you can come in all clean and shiny in 0.96. src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12796 > The package in first class is protobuf.generated. Here its generated.protobuf. Did you mean that? src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12797 > What is one of these? A bit of a comment? src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12798 > ditto. Whats this map too? src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12799 > We used to 'guess' this from the bytes passed. You are changing that? src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12800 > And we'd generate the encoded name from these articles? src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12801 > uint32 seems wide for number of stores src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12802 > ditto src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12803 > This is ServerName? Why call it ServerSpecifier? It should be a different name? src/main/proto/hbase.proto < https://reviews.apache.org/r/4283/#comment12804 > Should ColumnMetadata and TableMetadata have common type? Michael On 2012-03-10 02:09:45, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4283/ ----------------------------------------------------------- (Updated 2012-03-10 02:09:45) Review request for hbase. Summary ------- First draft of the protobufs specification for HMasterInterface. This is relatively close to a one-to-one mapping with the existing interface. A pdf listing the existing-to-protobufs mapping is available on the JIRA: https://issues.apache.org/jira/browse/HBASE-5445 Thanks to Devaraj who provided some initial work he had done on this. This addresses bug HBASE-5445 . https://issues.apache.org/jira/browse/HBASE-5445 Diffs ----- src/main/proto/HMasterProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4283/diff Testing ------- Thanks, Gregory
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > Looks good. Just a few comments below.

        Thanks for the review. Will address in next patch.

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/HMasterProtocol.proto, line 85

        > <https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line85>

        >

        > These are in the master protocol but are they used elsewhere? If so, should they be in here? Maybe they are not and its just me confused.

        I don't think they are used elsewhere, will check.

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/HMasterProtocol.proto, line 112

        > <https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line112>

        >

        > Is this class deprecated (HServerInfo?)

        This is equivalent to HServerLoad.

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/hbase.proto, line 21

        > <https://reviews.apache.org/r/4283/diff/1/?file=91003#file91003line21>

        >

        > The package in first class is protobuf.generated. Here its generated.protobuf. Did you mean that?

        Good catch.

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/hbase.proto, line 119

        > <https://reviews.apache.org/r/4283/diff/1/?file=91003#file91003line119>

        >

        > This is ServerName? Why call it ServerSpecifier? It should be a different name?

        I was just matching it up so it would be consistent with RegionSpecifier – I'll figure out something better for this.

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/HMasterProtocol.proto, line 22

        > <https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line22>

        >

        > Is this a good name for this class? Drop the H? Can we change it?

        I'll drop the H everywhere I can .

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/HMasterProtocol.proto, line 19

        > <https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line19>

        >

        > Reference your nice PDF?

        >

        > BTW, in your PDF, you have one method that is 'not used' so you don't include it. Do we need to deprecate it in 0.94 so its safe to remove by the time 0.96 comes around? If so, mind filing an issue or just me know and I can take care of it.

        >

        > Ditto for loadBalancerSwitchIs(LoadBalancerSwitchIsRequest). This method is just broke from its name and to how it works. Can we deprecate it in 0.94? Add a method that makes sense in 0.94, one that makes sense that you can redo in pb?

        >

        > Otherwise, the pdf looks good.

        I got rid of "public void assign(final byte [] regionName, final boolean force)" which is already deprecated, so nothing to do for 0.94.

        Do you have any recommendations for:
        "public boolean balanceSwitch(final boolean b)" and "public boolean synchronousBalanceSwitch(final boolean b)"?
        which is what I combined into loadBalancerSwitchIs. I thought "switch" was confusing because it is unclear if that is a verb (i.e. it switches whether the load balancer is running, so non-idempotent), vs a noun (it is a switch like an oven switch [on/off], so idempotent). I think a noun is the correct way to think about it; I thought "SwitchIs" makes it clearer that its a noun. On second though, I think I'd just call it "loadBalancerIs(final bool on,...)." Should I add that method and deprecate the existing two methods above in 0.94? I thought we were going to break everything in 0.96 anyway?

        On 2012-03-13 05:33:05, Michael Stack wrote:

        > src/main/proto/hbase.proto, line 149

        > <https://reviews.apache.org/r/4283/diff/1/?file=91003#file91003line149>

        >

        > Should ColumnMetadata and TableMetadata have common type?

        They could be. Having them separate lets us evolve them independently, though.

        • Gregory

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4283/#review5868
        -----------------------------------------------------------

        On 2012-03-10 02:09:45, Gregory Chanan wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4283/

        -----------------------------------------------------------

        (Updated 2012-03-10 02:09:45)

        Review request for hbase.

        Summary

        -------

        First draft of the protobufs specification for HMasterInterface.

        This is relatively close to a one-to-one mapping with the existing interface. A pdf listing the existing-to-protobufs mapping is available on the JIRA: https://issues.apache.org/jira/browse/HBASE-5445

        Thanks to Devaraj who provided some initial work he had done on this.

        This addresses bug HBASE-5445.

        https://issues.apache.org/jira/browse/HBASE-5445

        Diffs

        -----

        src/main/proto/HMasterProtocol.proto PRE-CREATION

        src/main/proto/hbase.proto PRE-CREATION

        Diff: https://reviews.apache.org/r/4283/diff

        Testing

        -------

        Thanks,

        Gregory

        Show
        jiraposter@reviews.apache.org added a comment - On 2012-03-13 05:33:05, Michael Stack wrote: > Looks good. Just a few comments below. Thanks for the review. Will address in next patch. On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/HMasterProtocol.proto, line 85 > < https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line85 > > > These are in the master protocol but are they used elsewhere? If so, should they be in here? Maybe they are not and its just me confused. I don't think they are used elsewhere, will check. On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/HMasterProtocol.proto, line 112 > < https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line112 > > > Is this class deprecated (HServerInfo?) This is equivalent to HServerLoad. On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/hbase.proto, line 21 > < https://reviews.apache.org/r/4283/diff/1/?file=91003#file91003line21 > > > The package in first class is protobuf.generated. Here its generated.protobuf. Did you mean that? Good catch. On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/hbase.proto, line 119 > < https://reviews.apache.org/r/4283/diff/1/?file=91003#file91003line119 > > > This is ServerName? Why call it ServerSpecifier? It should be a different name? I was just matching it up so it would be consistent with RegionSpecifier – I'll figure out something better for this. On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/HMasterProtocol.proto, line 22 > < https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line22 > > > Is this a good name for this class? Drop the H? Can we change it? I'll drop the H everywhere I can . On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/HMasterProtocol.proto, line 19 > < https://reviews.apache.org/r/4283/diff/1/?file=91002#file91002line19 > > > Reference your nice PDF? > > BTW, in your PDF, you have one method that is 'not used' so you don't include it. Do we need to deprecate it in 0.94 so its safe to remove by the time 0.96 comes around? If so, mind filing an issue or just me know and I can take care of it. > > Ditto for loadBalancerSwitchIs(LoadBalancerSwitchIsRequest). This method is just broke from its name and to how it works. Can we deprecate it in 0.94? Add a method that makes sense in 0.94, one that makes sense that you can redo in pb? > > Otherwise, the pdf looks good. I got rid of "public void assign(final byte [] regionName, final boolean force)" which is already deprecated, so nothing to do for 0.94. Do you have any recommendations for: "public boolean balanceSwitch(final boolean b)" and "public boolean synchronousBalanceSwitch(final boolean b)"? which is what I combined into loadBalancerSwitchIs. I thought "switch" was confusing because it is unclear if that is a verb (i.e. it switches whether the load balancer is running, so non-idempotent), vs a noun (it is a switch like an oven switch [on/off] , so idempotent). I think a noun is the correct way to think about it; I thought "SwitchIs" makes it clearer that its a noun. On second though, I think I'd just call it "loadBalancerIs(final bool on,...)." Should I add that method and deprecate the existing two methods above in 0.94? I thought we were going to break everything in 0.96 anyway? On 2012-03-13 05:33:05, Michael Stack wrote: > src/main/proto/hbase.proto, line 149 > < https://reviews.apache.org/r/4283/diff/1/?file=91003#file91003line149 > > > Should ColumnMetadata and TableMetadata have common type? They could be. Having them separate lets us evolve them independently, though. Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4283/#review5868 ----------------------------------------------------------- On 2012-03-10 02:09:45, Gregory Chanan wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4283/ ----------------------------------------------------------- (Updated 2012-03-10 02:09:45) Review request for hbase. Summary ------- First draft of the protobufs specification for HMasterInterface. This is relatively close to a one-to-one mapping with the existing interface. A pdf listing the existing-to-protobufs mapping is available on the JIRA: https://issues.apache.org/jira/browse/HBASE-5445 Thanks to Devaraj who provided some initial work he had done on this. This addresses bug HBASE-5445 . https://issues.apache.org/jira/browse/HBASE-5445 Diffs ----- src/main/proto/HMasterProtocol.proto PRE-CREATION src/main/proto/hbase.proto PRE-CREATION Diff: https://reviews.apache.org/r/4283/diff Testing ------- Thanks, Gregory
        Hide
        Gregory Chanan added a comment -

        Marking as resolved as all subtasks are done.

        Show
        Gregory Chanan added a comment - Marking as resolved as all subtasks are done.
        Hide
        stack added a comment -

        Marking closed.

        Show
        stack added a comment - Marking closed.

          People

          • Assignee:
            Gregory Chanan
            Reporter:
            Todd Lipcon
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development