Re: Review Request 34361: converted hard-coded strings to consts

2015-08-24 Thread Niklas Nielsen


 On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
 
 Niklas Nielsen wrote:
 Okay - thinking about this; I am on board with key1, value1 etc.
 
 Colin: the tech debt is that we have some inlined constants (like foo, 
 bar, etc) and some which are constants (which have to be kept in sync 
 between hooks modules and test body).
 The idea was to unify the way we name the key value pairs.
 
 Do you want to update this ticket to reflect this, or come with a new 
 patch set which tackles streaming the two?
 
 Niklas Nielsen wrote:
 Ping :)
 
 Colin Williams wrote:
 Sorry, my day job's been really all-consuming the last few days. I was 
 planning to update the ticket.
 
 Colin Williams wrote:
 Ticket updated
 
 Niklas Nielsen wrote:
 Colin; thanks! should we keep this patch then, or will you be providing a 
 new one?
 
 Colin Williams wrote:
 I'm not planning on providing a new one.

Sorry for the long turn around time, Colin!

To BenM's point; let's just rename to key1,value1 consistently instead of just 
hoisting the constants inline. Would you be up for doing this? We can land that 
immediately.


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 12:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   

Re: Review Request 34361: converted hard-coded strings to consts

2015-07-16 Thread Colin Williams


 On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
 
 Niklas Nielsen wrote:
 Okay - thinking about this; I am on board with key1, value1 etc.
 
 Colin: the tech debt is that we have some inlined constants (like foo, 
 bar, etc) and some which are constants (which have to be kept in sync 
 between hooks modules and test body).
 The idea was to unify the way we name the key value pairs.
 
 Do you want to update this ticket to reflect this, or come with a new 
 patch set which tackles streaming the two?
 
 Niklas Nielsen wrote:
 Ping :)
 
 Colin Williams wrote:
 Sorry, my day job's been really all-consuming the last few days. I was 
 planning to update the ticket.
 
 Colin Williams wrote:
 Ticket updated
 
 Niklas Nielsen wrote:
 Colin; thanks! should we keep this patch then, or will you be providing a 
 new one?

I'm not planning on providing a new one.


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-07-16 Thread Niklas Nielsen


 On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
 
 Niklas Nielsen wrote:
 Okay - thinking about this; I am on board with key1, value1 etc.
 
 Colin: the tech debt is that we have some inlined constants (like foo, 
 bar, etc) and some which are constants (which have to be kept in sync 
 between hooks modules and test body).
 The idea was to unify the way we name the key value pairs.
 
 Do you want to update this ticket to reflect this, or come with a new 
 patch set which tackles streaming the two?
 
 Niklas Nielsen wrote:
 Ping :)
 
 Colin Williams wrote:
 Sorry, my day job's been really all-consuming the last few days. I was 
 planning to update the ticket.
 
 Colin Williams wrote:
 Ticket updated

Colin; thanks! should we keep this patch then, or will you be providing a new 
one?


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 12:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-22 Thread Colin Williams


 On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
 
 Niklas Nielsen wrote:
 Okay - thinking about this; I am on board with key1, value1 etc.
 
 Colin: the tech debt is that we have some inlined constants (like foo, 
 bar, etc) and some which are constants (which have to be kept in sync 
 between hooks modules and test body).
 The idea was to unify the way we name the key value pairs.
 
 Do you want to update this ticket to reflect this, or come with a new 
 patch set which tackles streaming the two?
 
 Niklas Nielsen wrote:
 Ping :)
 
 Colin Williams wrote:
 Sorry, my day job's been really all-consuming the last few days. I was 
 planning to update the ticket.

Ticket updated


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-15 Thread Niklas Nielsen


 On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
 
 Niklas Nielsen wrote:
 Okay - thinking about this; I am on board with key1, value1 etc.
 
 Colin: the tech debt is that we have some inlined constants (like foo, 
 bar, etc) and some which are constants (which have to be kept in sync 
 between hooks modules and test body).
 The idea was to unify the way we name the key value pairs.
 
 Do you want to update this ticket to reflect this, or come with a new 
 patch set which tackles streaming the two?

Ping :)


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 12:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-15 Thread Colin Williams


 On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
 
 Niklas Nielsen wrote:
 Okay - thinking about this; I am on board with key1, value1 etc.
 
 Colin: the tech debt is that we have some inlined constants (like foo, 
 bar, etc) and some which are constants (which have to be kept in sync 
 between hooks modules and test body).
 The idea was to unify the way we name the key value pairs.
 
 Do you want to update this ticket to reflect this, or come with a new 
 patch set which tackles streaming the two?
 
 Niklas Nielsen wrote:
 Ping :)

Sorry, my day job's been really all-consuming the last few days. I was planning 
to update the ticket.


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-11 Thread Niklas Nielsen


 On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.
 
 Colin Williams wrote:
 Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
 point of unifying label names into a centralized variable that aren't at all 
 related.
 
 Niklas: I was a little confused by the original task description, so I 
 thought a sample patch would be a good discussion point. I don't entirely 
 understand the tech debt you're trying to solve. For example, it seems like 
 you're asking to have some constants defined somewhere that are used by both 
 master_test and slave_test, but the the similarities between these two only 
 seem incidental. I'm concerned that creating something like 
 CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.

Okay - thinking about this; I am on board with key1, value1 etc.

Colin: the tech debt is that we have some inlined constants (like foo, bar, 
etc) and some which are constants (which have to be kept in sync between hooks 
modules and test body).
The idea was to unify the way we name the key value pairs.

Do you want to update this ticket to reflect this, or come with a new patch set 
which tackles streaming the two?


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 12:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-11 Thread Colin Williams


 On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?
 
 Niklas Nielsen wrote:
 Try to see how testLabelKey and testLabelValue was used previously and 
 foo, bar, qux was used on others and I created this ticket to unify the 
 way we do this, along with seeing these key value pair being created in the 
 slave and master tests.
 
 Dispite the current patch, I do think we can simply and unify the test 
 label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
 which key pairs are being tested. The comments in the test code need to carry 
 a bunch of context, to make sense out of the label transformations 
 (especially across the library border for the hooks tests).
 
 This is all in spirit of reducing the tech debt we introduced. We are on 
 the same page on not introducing more lines/key pairs than before. Let us 
 iterate on this and get back to you.

Ben: I'm more in agreement with your sentiment, I'm not sure I see the point of 
unifying label names into a centralized variable that aren't at all related.

Niklas: I was a little confused by the original task description, so I thought 
a sample patch would be a good discussion point. I don't entirely understand 
the tech debt you're trying to solve. For example, it seems like you're asking 
to have some constants defined somewhere that are used by both master_test and 
slave_test, but the the similarities between these two only seem incidental. 
I'm concerned that creating something like CREATE_LABEL_PAIR(id) would instead 
obfuscate the intent of the code.


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-10 Thread Ben Mahler


 On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?

Well, then let's just keep it simple and get rid of these special strings by 
just making the tests use key1, value1, key2, value2, etc.

I'm not following your code duplication point, this introduces more code (now 
there are additional global constants, which we like to avoid if unnecessary).

Also, each test is ideally independent, why does the master's test for labels 
affect the slave's test for labels? Let's say I need a test with 5 labels, you 
want to have 5x2=10 global constants to address this?


- Ben


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-10 Thread Niklas Nielsen


 On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.
 
 Niklas Nielsen wrote:
 Colin: exactly
 
 Ben: We have label tests three places; in the master, slave and in the 
 modules and they use the same foo, bar, baz, qux key value pairs.
 The idea was to centralize them, so they don't get out of date and to 
 avoid code duplication.
 
 Does that make sense?
 
 Ben Mahler wrote:
 Well, then let's just keep it simple and get rid of these special strings 
 by just making the tests use key1, value1, key2, value2, etc.
 
 I'm not following your code duplication point, this introduces more code 
 (now there are additional global constants, which we like to avoid if 
 unnecessary).
 
 Also, each test is ideally independent, why does the master's test for 
 labels affect the slave's test for labels? Let's say I need a test with 5 
 labels, you want to have 5x2=10 global constants to address this?

Try to see how testLabelKey and testLabelValue was used previously and foo, 
bar, qux was used on others and I created this ticket to unify the way we 
do this, along with seeing these key value pair being created in the slave and 
master tests.

Dispite the current patch, I do think we can simply and unify the test label 
creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which 
key pairs are being tested. The comments in the test code need to carry a bunch 
of context, to make sense out of the label transformations (especially across 
the library border for the hooks tests).

This is all in spirit of reducing the tech debt we introduced. We are on the 
same page on not introducing more lines/key pairs than before. Let us iterate 
on this and get back to you.


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 12:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-09 Thread Colin Williams


 On June 9, 2015, 6:19 p.m., Niklas Nielsen wrote:
  Hey Colin!
  
  Thanks for contributing this and I am sorry about the tardy turn-around 
  time.
  
  The crux of the ticket was to consolidate the strings, so we only have to 
  maintain them one place.
  With that in mind; instead of inlining the string constants, can we add 
  them to an appropiate header as 'const char[]'?
  
  @Vinod: Would it make sense to add this to src/tests/mesos.hpp and 
  src/tests/mesos.cpp, as the label tests are in module, master and slave 
  tests?

It made sense to me to connect the values that were actually related, but it 
seemed like many of these strings shared a value only because foo and bar 
are really common test values. I thought it would be misleading to consolidate 
the strings for which it didn't matter if they matched, am I missing a 
connection somewhere?


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-09 Thread Colin Williams


 On June 9, 2015, 6:19 p.m., Niklas Nielsen wrote:
  Hey Colin!
  
  Thanks for contributing this and I am sorry about the tardy turn-around 
  time.
  
  The crux of the ticket was to consolidate the strings, so we only have to 
  maintain them one place.
  With that in mind; instead of inlining the string constants, can we add 
  them to an appropiate header as 'const char[]'?
  
  @Vinod: Would it make sense to add this to src/tests/mesos.hpp and 
  src/tests/mesos.cpp, as the label tests are in module, master and slave 
  tests?
 
 Colin Williams wrote:
 It made sense to me to connect the values that were actually related, but 
 it seemed like many of these strings shared a value only because foo and 
 bar are really common test values. I thought it would be misleading to 
 consolidate the strings for which it didn't matter if they matched, am I 
 missing a connection somewhere?

Actually, there seemed to be a connection between src/tests/hook_tests.cpp and 
src/examples/test_hook_modules.cpp, but I couldn't find it in the code. It also 
looked like the convention had already been established between those files of 
keeping them in sync with a series of variable declarations at the top of each 
file. There was some negative feedback with those changes, though, so I 
reverted them.


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 7:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-06-09 Thread Niklas Nielsen


 On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
  src/tests/master_tests.cpp, lines 3031-3034
  https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031
 
  Why bother with all this? Why not just have `key1`, `value1`, 
  `key2`, `value2` inlined appropriately throughout these tests. Very 
  straightforward to read.
 
 Colin Williams wrote:
 I think the issue with the changes remaining is that the test depends on 
 the same value occurring in several places. By consolidating to a variable 
 it's no longer possible for these values to get out of sync.

Colin: exactly

Ben: We have label tests three places; in the master, slave and in the modules 
and they use the same foo, bar, baz, qux key value pairs.
The idea was to centralize them, so they don't get out of date and to avoid 
code duplication.

Does that make sense?


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated June 8, 2015, 12:05 p.m.)
 
 
 Review request for mesos and Niklas Nielsen.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-26 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [34361]

All tests passed.

- Mesos ReviewBot


On May 25, 2015, 6:59 a.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 25, 2015, 6:59 a.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-25 Thread Colin Williams

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

(Updated May 25, 2015, 6:59 a.m.)


Review request for mesos.


Bugs: MESOS-2637
https://issues.apache.org/jira/browse/MESOS-2637


Repository: mesos


Description
---

converted hard-coded strings to consts


Diffs (updated)
-

  src/tests/hook_tests.cpp 3ffde6d 
  src/tests/master_tests.cpp ba3858f 
  src/tests/slave_tests.cpp acae497 

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


Testing
---


Thanks,

Colin Williams



Re: Review Request 34361: converted hard-coded strings to consts

2015-05-25 Thread Colin Williams


 On May 20, 2015, 11:47 a.m., Alexander Rojas wrote:
  src/examples/test_hook_module.cpp, lines 36-38
  https://reviews.apache.org/r/34361/diff/1/?file=962951#file962951line36
 
  This constants are used nowhere but in one method. Is there any reason 
  why they are not defined in the method itself or at least as static 
  attributes of the TestHook class?
 
 Colin Williams wrote:
 There's a mitigating circumstance for these declarations, there's a 
 comment a few lines up from the change instructing that these should be kept 
 in sync with the values in src/tests/hook_tests.cpp. Presumably they haven't 
 been pulled into a separate file because it would make the example less 
 self-contained. Bringing the names and types in sync with the ones created in 
 hook_tests.cpp, but leaving them where they are.
 
 Colin Williams wrote:
 Actually, can anybody tell why these need to be kept in sync? I don't see 
 any place in the code where these two files are connected.

Well, based on some experimentation, those variables clearly need to be kept in 
sync. For the ones I introduced, on the other hand, that doesn't seem to 
matter. In fact, in the case of test_hook_module.cpp the values aren't even 
repeated, so the variable doesn't really seem necessary.


- Colin


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


On May 25, 2015, 6:59 a.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 25, 2015, 6:59 a.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-25 Thread Colin Williams


 On May 18, 2015, 5:48 p.m., Marco Massenzio wrote:
  src/examples/test_hook_module.cpp, lines 36-38
  https://reviews.apache.org/r/34361/diff/1/?file=962951#file962951line36
 
  Thanks for doing this!
  
  I'm wondering whether, as these are constants, shouldn't they be in 
  `SCREAMING_SNAKE_CASE` as per the [Style 
  Guide](http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/)?
  
  I completely realize that the existing code (lines above) does not 
  follow this style, so I'm really wondering whether that was by design or 
  accident?
  
  And, if the latter, would this be also a good time to fix those too?
  
  No matter what, though, this is A Good Thing.
 
 Colin Williams wrote:
 I'd be fine converting the other constants too, I guess just waiting on 
 somebody saying if it was intentional or not.

Based on other feedback, these consts ended up getting localized to bring them 
closer to the place where they were needed. That makes this point moot.


- Colin


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


On May 25, 2015, 6:59 a.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 25, 2015, 6:59 a.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/tests/hook_tests.cpp 3ffde6d 
   src/tests/master_tests.cpp ba3858f 
   src/tests/slave_tests.cpp acae497 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-23 Thread Colin Williams

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

(Updated May 23, 2015, 8:02 p.m.)


Review request for mesos.


Changes
---

I'm not sure why that change in test_hook_module is showing up in this diff, it 
doesn't show up in the patch file.


Bugs: MESOS-2637
https://issues.apache.org/jira/browse/MESOS-2637


Repository: mesos


Description
---

converted hard-coded strings to consts


Diffs (updated)
-

  src/examples/test_hook_module.cpp d61cd55 
  src/tests/hook_tests.cpp 3ffde6d 
  src/tests/master_tests.cpp ba3858f 
  src/tests/slave_tests.cpp acae497 

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


Testing
---


Thanks,

Colin Williams



Re: Review Request 34361: converted hard-coded strings to consts

2015-05-21 Thread Colin Williams


 On May 20, 2015, 11:47 a.m., Alexander Rojas wrote:
  src/examples/test_hook_module.cpp, lines 36-38
  https://reviews.apache.org/r/34361/diff/1/?file=962951#file962951line36
 
  This constants are used nowhere but in one method. Is there any reason 
  why they are not defined in the method itself or at least as static 
  attributes of the TestHook class?

There's a mitigating circumstance for these declarations, there's a comment a 
few lines up from the change instructing that these should be kept in sync with 
the values in src/tests/hook_tests.cpp. Presumably they haven't been pulled 
into a separate file because it would make the example less self-contained. 
Bringing the names and types in sync with the ones created in hook_tests.cpp, 
but leaving them where they are.


 On May 20, 2015, 11:47 a.m., Alexander Rojas wrote:
  src/tests/hook_tests.cpp, lines 74-77
  https://reviews.apache.org/r/34361/diff/1/?file=962952#file962952line74
 
  ditto.
  
  Now there are 200 lines between definition and usage. If I am debugging 
  or just checking the code, I'll have to scroll all the way up to see the 
  values.

ditto


- Colin


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


On May 18, 2015, 5:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 5:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-21 Thread Colin Williams


 On May 20, 2015, 11:47 a.m., Alexander Rojas wrote:
  src/examples/test_hook_module.cpp, lines 36-38
  https://reviews.apache.org/r/34361/diff/1/?file=962951#file962951line36
 
  This constants are used nowhere but in one method. Is there any reason 
  why they are not defined in the method itself or at least as static 
  attributes of the TestHook class?
 
 Colin Williams wrote:
 There's a mitigating circumstance for these declarations, there's a 
 comment a few lines up from the change instructing that these should be kept 
 in sync with the values in src/tests/hook_tests.cpp. Presumably they haven't 
 been pulled into a separate file because it would make the example less 
 self-contained. Bringing the names and types in sync with the ones created in 
 hook_tests.cpp, but leaving them where they are.

Actually, can anybody tell why these need to be kept in sync? I don't see any 
place in the code where these two files are connected.


- Colin


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


On May 18, 2015, 5:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 5:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-20 Thread Alexander Rojas

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



src/examples/test_hook_module.cpp
https://reviews.apache.org/r/34361/#comment135810

This constants are used nowhere but in one method. Is there any reason why 
they are not defined in the method itself or at least as static attributes of 
the TestHook class?



src/tests/hook_tests.cpp
https://reviews.apache.org/r/34361/#comment135811

ditto.

Now there are 200 lines between definition and usage. If I am debugging or 
just checking the code, I'll have to scroll all the way up to see the values.



src/tests/master_tests.cpp
https://reviews.apache.org/r/34361/#comment135812

Ditto.

Now 2000 lines between definition and first usage.



src/tests/slave_tests.cpp
https://reviews.apache.org/r/34361/#comment135813

ditto.


- Alexander Rojas


On May 18, 2015, 7:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 7:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-20 Thread Colin Williams


 On May 20, 2015, 11:47 a.m., Alexander Rojas wrote:
 

This is a very good point, my original change had the values set as local 
variables, but when I read through the task again it seemed like it was asking 
for predefined constants. I'll make the changes and put them up during my 
morning commute tomorrow.


- Colin


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


On May 18, 2015, 5:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 5:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-18 Thread Marco Massenzio

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



src/examples/test_hook_module.cpp
https://reviews.apache.org/r/34361/#comment135300

Thanks for doing this!

I'm wondering whether, as these are constants, shouldn't they be in 
`SCREAMING_SNAKE_CASE` as per the [Style 
Guide](http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/)?

I completely realize that the existing code (lines above) does not follow 
this style, so I'm really wondering whether that was by design or accident?

And, if the latter, would this be also a good time to fix those too?

No matter what, though, this is A Good Thing.


- Marco Massenzio


On May 18, 2015, 5:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 5:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-18 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [34361]

All tests passed.

- Mesos ReviewBot


On May 18, 2015, 5:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 5:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams
 




Re: Review Request 34361: converted hard-coded strings to consts

2015-05-18 Thread Colin Williams


 On May 18, 2015, 5:48 p.m., Marco Massenzio wrote:
  src/examples/test_hook_module.cpp, lines 36-38
  https://reviews.apache.org/r/34361/diff/1/?file=962951#file962951line36
 
  Thanks for doing this!
  
  I'm wondering whether, as these are constants, shouldn't they be in 
  `SCREAMING_SNAKE_CASE` as per the [Style 
  Guide](http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/)?
  
  I completely realize that the existing code (lines above) does not 
  follow this style, so I'm really wondering whether that was by design or 
  accident?
  
  And, if the latter, would this be also a good time to fix those too?
  
  No matter what, though, this is A Good Thing.

I'd be fine converting the other constants too, I guess just waiting on 
somebody saying if it was intentional or not.


- Colin


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


On May 18, 2015, 5:01 p.m., Colin Williams wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/34361/
 ---
 
 (Updated May 18, 2015, 5:01 p.m.)
 
 
 Review request for mesos.
 
 
 Bugs: MESOS-2637
 https://issues.apache.org/jira/browse/MESOS-2637
 
 
 Repository: mesos
 
 
 Description
 ---
 
 converted hard-coded strings to consts
 
 
 Diffs
 -
 
   src/examples/test_hook_module.cpp b25830ab6475f997422cfd2f60cc9a79e1acadfe 
   src/tests/hook_tests.cpp a65c0ab7c41ec3b7964f9d572381fa3e61746dc3 
   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
   src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 
 
 Diff: https://reviews.apache.org/r/34361/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Colin Williams