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-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-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-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?
 
 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 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 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