Re: Review Request 34361: converted hard-coded strings to consts
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
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
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
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
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
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
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
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
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
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
--- 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
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
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
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