Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.8.1
    • Fix Version/s: 0.9.0
    • Labels:
      None

      Description

      It would be great if user could check custom counter values in addition to checking outputs. Let me show my idea on example, right now counters needs to be checked explicitly:

      assertEquals(2, mapDriver.getCounters().findCounter(CustomMapper.CustomCounter.NAME).getValue());

      It would be great if user could do something like:

      .withCounter(CustomMapper.CustomCounter.Name, 2)

      1. MRUNIT-68.patch
        17 kB
        Jarek Jarcec Cecho
      2. MRUNIT-68.patch
        38 kB
        Jarek Jarcec Cecho
      3. MRUNIT-68.patch
        40 kB
        Jarek Jarcec Cecho
      4. MRUNIT-68.patch
        38 kB
        Jarek Jarcec Cecho

        Activity

        Hide
        Jarek Jarcec Cecho added a comment -

        Thank you sir for removing the unnecessary 0 counter size check.

        I actually see the "inconsistency" with renaming the method to addCounter. It looks cleaner to me using withCounter method name. But I'm not strong on this opinion, so really feel free to rename it.

        Show
        Jarek Jarcec Cecho added a comment - Thank you sir for removing the unnecessary 0 counter size check. I actually see the "inconsistency" with renaming the method to addCounter. It looks cleaner to me using withCounter method name. But I'm not strong on this opinion, so really feel free to rename it.
        Hide
        Jim Donofrio added a comment -

        I removed the unnecessary 0 counter size check in rev 1302763.

        I dont understand how it would be consistent with the rest of the code if we changed the name to addCounter. All of the other add methods return void. If we changed the name to addCounter then it should also return void to be consistent.

        Show
        Jim Donofrio added a comment - I removed the unnecessary 0 counter size check in rev 1302763. I dont understand how it would be consistent with the rest of the code if we changed the name to addCounter. All of the other add methods return void. If we changed the name to addCounter then it should also return void to be consistent.
        Hide
        Jarek Jarcec Cecho added a comment -
        • Good point about the compiler error, you could change the name in TestDriver from withCounter to addCounter and then you wont have the compiler error

        I honestly prefer returning this in TestDriver even thought that return value is not used. It seems to me cleaner and more consistent with the rest of the code. However please feel free to rename the method.

        • One other suggestion is that the expected 0 counter size check is unnecessary at the beginning of validate, success will remain true and neither for loop will run if both are 0.

        No issues on my side at all. I've put the check there because I was following previous implementation of validate(expectedOutputs). My intention was just to be consistent.

        Show
        Jarek Jarcec Cecho added a comment - Good point about the compiler error, you could change the name in TestDriver from withCounter to addCounter and then you wont have the compiler error I honestly prefer returning this in TestDriver even thought that return value is not used. It seems to me cleaner and more consistent with the rest of the code. However please feel free to rename the method. One other suggestion is that the expected 0 counter size check is unnecessary at the beginning of validate, success will remain true and neither for loop will run if both are 0. No issues on my side at all. I've put the check there because I was following previous implementation of validate(expectedOutputs). My intention was just to be consistent.
        Hide
        Brock Noland added a comment -

        not from me!

        Show
        Brock Noland added a comment - not from me!
        Hide
        Jim Donofrio added a comment - - edited

        Are there any objections to me changing:

        Good point about the compiler error, you could change the name in TestDriver from withCounter to addCounter and then you wont have the compiler error

        One other suggestion is that the expected 0 counter size check is unnecessary at the beginning of validate, success will remain true and neither for loop will run if both are 0.

        Show
        Jim Donofrio added a comment - - edited Are there any objections to me changing: Good point about the compiler error, you could change the name in TestDriver from withCounter to addCounter and then you wont have the compiler error One other suggestion is that the expected 0 counter size check is unnecessary at the beginning of validate, success will remain true and neither for loop will run if both are 0.
        Hide
        Jarek Jarcec Cecho added a comment -

        Thank you guys for your reviews!

        Jarcec

        Show
        Jarek Jarcec Cecho added a comment - Thank you guys for your reviews! Jarcec
        Hide
        Brock Noland added a comment -

        OK, neither of those look like review items big enough to hold up a commit. Jim, if you feel differently we can continue to discuss this post commit since we have a CTR (commit then review) policy on this project.

        Committed in 1301944.

        Thank you for your contribution Jarek!

        Show
        Brock Noland added a comment - OK, neither of those look like review items big enough to hold up a commit. Jim, if you feel differently we can continue to discuss this post commit since we have a CTR (commit then review) policy on this project. Committed in 1301944. Thank you for your contribution Jarek!
        Hide
        Jim Donofrio added a comment -

        Ok sounds good.

        Good point about the compiler error, you could change the name in TestDriver from withCounter to addCounter and then you wont have the compiler error

        One other suggestion is that the expected 0 counter size check is unnecessary at the beginning of validate, success will remain true and neither for loop will run if both are 0.

        Show
        Jim Donofrio added a comment - Ok sounds good. Good point about the compiler error, you could change the name in TestDriver from withCounter to addCounter and then you wont have the compiler error One other suggestion is that the expected 0 counter size check is unnecessary at the beginning of validate, success will remain true and neither for loop will run if both are 0.
        Hide
        Brock Noland added a comment -

        Looks good to me! I'll wait to hear back from Jim.

        Show
        Brock Noland added a comment - Looks good to me! I'll wait to hear back from Jim.
        Hide
        Jarek Jarcec Cecho added a comment - - edited

        HI Jim and Brock,
        I've updated my patch with your suggestions, however not all of them were met. Please see my comments inline (sorry for the formatting, I'm not sure why Apache instance of JIRA is ignoring formatting tags):

        CounterWrapper should use private instance variables

        I've changed the type to explicit private.

        CounterWrapper can use else statements in the findCounterValue methods, since if one counters variable is null the other must not be

        Changed. I do agree that only one counter might be filled. My original intention with two if statements was to have defined default value that can be used for distinguishing case where the counter is not present at all.

        TestDriver's with methods dont need to return this, they should return void since the calling methods wont use that result

        Not changed. I do agree that return value in TestDriver.withCounter() is not used, however changing it to void will result in compile errors:

        [ERROR] /home/jarcec/projects/apache/mrunit/src/main/java/org/apache/hadoop/mrunit/MapReduceDriver.java:[381,49] withCounter(java.lang.String,java.lang.String,long) in org.apache.hadoop.mrunit.MapReduceDriver cannot override withCounter(java.lang.String,java.lang.String,long) in org.apache.hadoop.mrunit.TestDriver; attempting to use incompatible return type
        [ERROR] found   : org.apache.hadoop.mrunit.MapReduceDriver<K1,V1,K2,V2,K3,V3>
        [ERROR] required: void
        

        TestDriver's validate should use fail(msg) not RuntimeException

        Changed. Thank you for pointing this out. I've started working on the patch when we were still using RuntimeExceptions.

        You should use the ExpectedSuppliedException class in test mrunit to verify the exception message returned

        Changed as well.

        I think 0 would be a clearer default value

        I've removed default value completely.

        Show
        Jarek Jarcec Cecho added a comment - - edited HI Jim and Brock, I've updated my patch with your suggestions, however not all of them were met. Please see my comments inline (sorry for the formatting, I'm not sure why Apache instance of JIRA is ignoring formatting tags): CounterWrapper should use private instance variables I've changed the type to explicit private. CounterWrapper can use else statements in the findCounterValue methods, since if one counters variable is null the other must not be Changed. I do agree that only one counter might be filled. My original intention with two if statements was to have defined default value that can be used for distinguishing case where the counter is not present at all. TestDriver's with methods dont need to return this, they should return void since the calling methods wont use that result Not changed. I do agree that return value in TestDriver.withCounter() is not used, however changing it to void will result in compile errors: [ERROR] /home/jarcec/projects/apache/mrunit/src/main/java/org/apache/hadoop/mrunit/MapReduceDriver.java:[381,49] withCounter(java.lang. String ,java.lang. String , long ) in org.apache.hadoop.mrunit.MapReduceDriver cannot override withCounter(java.lang. String ,java.lang. String , long ) in org.apache.hadoop.mrunit.TestDriver; attempting to use incompatible return type [ERROR] found : org.apache.hadoop.mrunit.MapReduceDriver<K1,V1,K2,V2,K3,V3> [ERROR] required: void TestDriver's validate should use fail(msg) not RuntimeException Changed. Thank you for pointing this out. I've started working on the patch when we were still using RuntimeExceptions. You should use the ExpectedSuppliedException class in test mrunit to verify the exception message returned Changed as well. I think 0 would be a clearer default value I've removed default value completely.
        Hide
        Brock Noland added a comment -

        Looks good! I am +1 once when Jim's concerns are met.

        Show
        Brock Noland added a comment - Looks good! I am +1 once when Jim's concerns are met.
        Hide
        Jarek Jarcec Cecho added a comment -

        Thank you Jim for your comments, I'll incorporate them over the weekend.

        Show
        Jarek Jarcec Cecho added a comment - Thank you Jim for your comments, I'll incorporate them over the weekend.
        Hide
        Jim Donofrio added a comment -

        Overall the patch looks good except:

        CounterWrapper should use private instance variables
        CounterWrapper can use else statements in the findCounterValue methods, since if one counters variable is null the other must not be
        TestDriver's with methods dont need to return this, they should return void since the calling methods wont use that result
        TestDriver's validate should use fail(msg) not RuntimeException
        You should use the ExpectedSuppliedException class in test mrunit to verify the exception message returned
        I think 0 would be a clearer default value

        Show
        Jim Donofrio added a comment - Overall the patch looks good except: CounterWrapper should use private instance variables CounterWrapper can use else statements in the findCounterValue methods, since if one counters variable is null the other must not be TestDriver's with methods dont need to return this, they should return void since the calling methods wont use that result TestDriver's validate should use fail(msg) not RuntimeException You should use the ExpectedSuppliedException class in test mrunit to verify the exception message returned I think 0 would be a clearer default value
        Hide
        Jarek Jarcec Cecho added a comment -

        I'm attaching updated version of my patch that is just adding new class CounterWrapper. Sorry for the inconvenience.

        Show
        Jarek Jarcec Cecho added a comment - I'm attaching updated version of my patch that is just adding new class CounterWrapper. Sorry for the inconvenience.
        Hide
        Brock Noland added a comment -

        No worries! I'll checkout the updated patch when attached.

        Show
        Brock Noland added a comment - No worries! I'll checkout the updated patch when attached.
        Hide
        Jarek Jarcec Cecho added a comment -

        I'm sorry, I'll update the patch once I got home.

        Show
        Jarek Jarcec Cecho added a comment - I'm sorry, I'll update the patch once I got home.
        Hide
        Brock Noland added a comment -

        @Jerek,

        I don't see the class org.apache.hadoop.mrunit.counters.CounterWrapper in the patch?

        Show
        Brock Noland added a comment - @Jerek, I don't see the class org.apache.hadoop.mrunit.counters.CounterWrapper in the patch?
        Hide
        Brock Noland added a comment -

        I am looking at this now, I will let you know shortly my thoughts. Tons of tests!

        Show
        Brock Noland added a comment - I am looking at this now, I will let you know shortly my thoughts. Tons of tests!
        Hide
        Jarek Jarcec Cecho added a comment -

        That's fine sir, please take your time. I mean no rush here, I just wanted to ping so that this issue is not forgotten.

        Show
        Jarek Jarcec Cecho added a comment - That's fine sir, please take your time. I mean no rush here, I just wanted to ping so that this issue is not forgotten.
        Hide
        Jim Donofrio added a comment -

        Yes sorry I will be looking at this week at some point, been busy trying to figure out MRUNIT-69

        Show
        Jim Donofrio added a comment - Yes sorry I will be looking at this week at some point, been busy trying to figure out MRUNIT-69
        Hide
        Jarek Jarcec Cecho added a comment -

        Might I kindly ask for review of my patch?

        Show
        Jarek Jarcec Cecho added a comment - Might I kindly ask for review of my patch?
        Hide
        Jarek Jarcec Cecho added a comment -

        I'm attaching fully featured patch.I tried to load it on review board, but it seems that mrunit is not available there.

        Show
        Jarek Jarcec Cecho added a comment - I'm attaching fully featured patch.I tried to load it on review board, but it seems that mrunit is not available there.
        Hide
        Jarek Jarcec Cecho added a comment -

        Yup, I'll definitely grant Apache license for patch that will be in intended for commit.

        Jarcec

        Show
        Jarek Jarcec Cecho added a comment - Yup, I'll definitely grant Apache license for patch that will be in intended for commit. Jarcec
        Hide
        Brock Noland added a comment -

        I just added something to another JIRA. I noticed this: "Contributions intended for inclusion in ASF products (eg. patches, code) must be licensed to ASF under the terms of the Apache License. Other attachments (eg. log dumps, test cases) need not be."

        I take that to mean patches will be included at some point should have the ASF license designation.

        Show
        Brock Noland added a comment - I just added something to another JIRA. I noticed this: "Contributions intended for inclusion in ASF products (eg. patches, code) must be licensed to ASF under the terms of the Apache License. Other attachments (eg. log dumps, test cases) need not be." I take that to mean patches will be included at some point should have the ASF license designation.
        Hide
        Brock Noland added a comment -

        I am not an Apache process expert so I am not sure. I think you are probably correct though, we can look at it but not include it. If we couldn't look at the attachement, why would they let you upload anything? But who knows.

        The only comment I have is that we should be able to specify enum counters and string, string counters.

        Cheers,
        Brock

        Show
        Brock Noland added a comment - I am not an Apache process expert so I am not sure. I think you are probably correct though, we can look at it but not include it. If we couldn't look at the attachement, why would they let you upload anything? But who knows. The only comment I have is that we should be able to specify enum counters and string, string counters. Cheers, Brock
        Hide
        Jarek Jarcec Cecho added a comment -

        As far as I know, code that is not explicitly allowed to be included in Apache works can't be just included in Apache projects. I'm personally thinking about it as "I can't commit this code". I do not believe that you can't even look on that

        Reason why I've submitted the patch without granting Apache license is to put stress that is not ready to be committed. It's just feature preview and I'm strongly looking for any feedback prior finishing it.

        @Brock: Thanks for the support, I'll finish the patch.

        Jarcec

        Show
        Jarek Jarcec Cecho added a comment - As far as I know, code that is not explicitly allowed to be included in Apache works can't be just included in Apache projects. I'm personally thinking about it as "I can't commit this code". I do not believe that you can't even look on that Reason why I've submitted the patch without granting Apache license is to put stress that is not ready to be committed. It's just feature preview and I'm strongly looking for any feedback prior finishing it. @Brock: Thanks for the support, I'll finish the patch. Jarcec
        Hide
        Brock Noland added a comment -

        Also, I like the idea of this JIRA!

        Show
        Brock Noland added a comment - Also, I like the idea of this JIRA!
        Hide
        Brock Noland added a comment -

        Yep, I think Jim is correct.

        Show
        Brock Noland added a comment - Yep, I think Jim is correct.
        Hide
        Jim Donofrio added a comment -

        I am new to this Apache license stuff but you might need to reload upload the patch with the license checked before we are allowed to look at it?

        Show
        Jim Donofrio added a comment - I am new to this Apache license stuff but you might need to reload upload the patch with the license checked before we are allowed to look at it?
        Hide
        Jarek Jarcec Cecho added a comment -

        I've put together simple patch with this feature preview. I just implemented this for new API to show community what I had on my mind by this JIRA. It's definitely not suitable for committing at the moment.

        I would appreciate any type of feedback. Would something like this be acceptable by the upstream?

        Show
        Jarek Jarcec Cecho added a comment - I've put together simple patch with this feature preview. I just implemented this for new API to show community what I had on my mind by this JIRA. It's definitely not suitable for committing at the moment. I would appreciate any type of feedback. Would something like this be acceptable by the upstream?

          People

          • Assignee:
            Jarek Jarcec Cecho
            Reporter:
            Jarek Jarcec Cecho
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development