Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96377 --- Ship it! Ship It! - Zameer Manji On Aug. 25, 2015, 8:01 a.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 8:01 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96362 --- Ship it! Ship It! - Maxim Khutornenko On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Review Request 37761: Add a link to the instance page from instance events on the update page.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96399 --- Master (86a547b) is green with this patch. ./build-support/jenkins/build.sh However, it appears that it might lack test coverage. I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96394 --- What if they click on an old update and the instance page doesn't reflect the change made in this instance event? Do we care? - David McLaughlin On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
On Aug. 25, 2015, 6:42 p.m., David McLaughlin wrote: What if they click on an old update and the instance page doesn't reflect the change made in this instance event? Do we care? I don't think we care, there's not much we can do in that case, is there? The only thing I can think of is disabling links for completed updates, but that seems overly broad (some completed updates will link to instances that still exist). - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96394 --- On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
On Aug. 25, 2015, 6:42 p.m., David McLaughlin wrote: What if they click on an old update and the instance page doesn't reflect the change made in this instance event? Do we care? Joshua Cohen wrote: I don't think we care, there's not much we can do in that case, is there? The only thing I can think of is disabling links for completed updates, but that seems overly broad (some completed updates will link to instances that still exist). David McLaughlin wrote: Right. I'm just concerned people click on the link to see what happened on that instance event. I know it's what I would expect, given it's the only link and the first column on the row too. Basically it comes down - the instance page you added is a 'live' view of that instance. It is potentially misleading to include that link on a table that deals exclusively with historical data. Yeah, I understand that, I'm just not sure what the alternative is other than not linking at all (which seems worse to me)? As far as I can see there's no association between an instance in an update and an actual task id (which would let us query to see if the scheduler still has a record of that task existing before displaying the link). That said, even if we *could* conditionally display the link, that might be even more confusing as it would be feasible that only *some* tasks from a historical update have been purged, while others might still remain, leading to a strange inconsistency on the update page where only some instances are links, while others are not. As far as I see it we have three options: 1) Always link to the instance page. 2) Only link to the instance page for active updates. 3) Never link to the instance page. Option 1 seems like the best option in that it provides an easy way to see what happened for an update, and in most likely cases (debugging an active or recently completed update) should provide useful data (though admittedly could prove to be confusing in the cases where the task that was part of an update has already been pruned). - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96394 --- On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
On Aug. 25, 2015, 6:42 p.m., David McLaughlin wrote: What if they click on an old update and the instance page doesn't reflect the change made in this instance event? Do we care? Joshua Cohen wrote: I don't think we care, there's not much we can do in that case, is there? The only thing I can think of is disabling links for completed updates, but that seems overly broad (some completed updates will link to instances that still exist). David McLaughlin wrote: Right. I'm just concerned people click on the link to see what happened on that instance event. I know it's what I would expect, given it's the only link and the first column on the row too. Basically it comes down - the instance page you added is a 'live' view of that instance. It is potentially misleading to include that link on a table that deals exclusively with historical data. Joshua Cohen wrote: Yeah, I understand that, I'm just not sure what the alternative is other than not linking at all (which seems worse to me)? As far as I can see there's no association between an instance in an update and an actual task id (which would let us query to see if the scheduler still has a record of that task existing before displaying the link). That said, even if we *could* conditionally display the link, that might be even more confusing as it would be feasible that only *some* tasks from a historical update have been purged, while others might still remain, leading to a strange inconsistency on the update page where only some instances are links, while others are not. As far as I see it we have three options: 1) Always link to the instance page. 2) Only link to the instance page for active updates. 3) Never link to the instance page. Option 1 seems like the best option in that it provides an easy way to see what happened for an update, and in most likely cases (debugging an active or recently completed update) should provide useful data (though admittedly could prove to be confusing in the cases where the task that was part of an update has already been pruned). David McLaughlin wrote: I'm leaning towards (3) but maybe that's because I don't understand where the requirement comes from. The linked ticket suggests the user story is I see an instance event with a 'bad' event status and I want to click straight through to see why.' I see the main use case for this occuring when you have a bad deploy, but if rollbacks are enabled you'd click the event and get taken to the rolled back instance page. This could be frustrating at best and misleading/confusing at worst. Happy to be overruled by a tie-breaking vote here. | but if rollbacks are enabled you'd click the event and get taken to the rolled back instance page. The instance page aggregates task history by instance ID, right? This is a perfect example when having instance history is actually quite useful when debugging a rollback. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96394 --- On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
On Aug. 25, 2015, 6:42 p.m., David McLaughlin wrote: What if they click on an old update and the instance page doesn't reflect the change made in this instance event? Do we care? Joshua Cohen wrote: I don't think we care, there's not much we can do in that case, is there? The only thing I can think of is disabling links for completed updates, but that seems overly broad (some completed updates will link to instances that still exist). David McLaughlin wrote: Right. I'm just concerned people click on the link to see what happened on that instance event. I know it's what I would expect, given it's the only link and the first column on the row too. Basically it comes down - the instance page you added is a 'live' view of that instance. It is potentially misleading to include that link on a table that deals exclusively with historical data. Joshua Cohen wrote: Yeah, I understand that, I'm just not sure what the alternative is other than not linking at all (which seems worse to me)? As far as I can see there's no association between an instance in an update and an actual task id (which would let us query to see if the scheduler still has a record of that task existing before displaying the link). That said, even if we *could* conditionally display the link, that might be even more confusing as it would be feasible that only *some* tasks from a historical update have been purged, while others might still remain, leading to a strange inconsistency on the update page where only some instances are links, while others are not. As far as I see it we have three options: 1) Always link to the instance page. 2) Only link to the instance page for active updates. 3) Never link to the instance page. Option 1 seems like the best option in that it provides an easy way to see what happened for an update, and in most likely cases (debugging an active or recently completed update) should provide useful data (though admittedly could prove to be confusing in the cases where the task that was part of an update has already been pruned). I'm leaning towards (3) but maybe that's because I don't understand where the requirement comes from. The linked ticket suggests the user story is I see an instance event with a 'bad' event status and I want to click straight through to see why.' I see the main use case for this occuring when you have a bad deploy, but if rollbacks are enabled you'd click the event and get taken to the rolled back instance page. This could be frustrating at best and misleading/confusing at worst. Happy to be overruled by a tie-breaking vote here. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96394 --- On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
On Aug. 25, 2015, 6:42 p.m., David McLaughlin wrote: What if they click on an old update and the instance page doesn't reflect the change made in this instance event? Do we care? Joshua Cohen wrote: I don't think we care, there's not much we can do in that case, is there? The only thing I can think of is disabling links for completed updates, but that seems overly broad (some completed updates will link to instances that still exist). David McLaughlin wrote: Right. I'm just concerned people click on the link to see what happened on that instance event. I know it's what I would expect, given it's the only link and the first column on the row too. Basically it comes down - the instance page you added is a 'live' view of that instance. It is potentially misleading to include that link on a table that deals exclusively with historical data. Joshua Cohen wrote: Yeah, I understand that, I'm just not sure what the alternative is other than not linking at all (which seems worse to me)? As far as I can see there's no association between an instance in an update and an actual task id (which would let us query to see if the scheduler still has a record of that task existing before displaying the link). That said, even if we *could* conditionally display the link, that might be even more confusing as it would be feasible that only *some* tasks from a historical update have been purged, while others might still remain, leading to a strange inconsistency on the update page where only some instances are links, while others are not. As far as I see it we have three options: 1) Always link to the instance page. 2) Only link to the instance page for active updates. 3) Never link to the instance page. Option 1 seems like the best option in that it provides an easy way to see what happened for an update, and in most likely cases (debugging an active or recently completed update) should provide useful data (though admittedly could prove to be confusing in the cases where the task that was part of an update has already been pruned). David McLaughlin wrote: I'm leaning towards (3) but maybe that's because I don't understand where the requirement comes from. The linked ticket suggests the user story is I see an instance event with a 'bad' event status and I want to click straight through to see why.' I see the main use case for this occuring when you have a bad deploy, but if rollbacks are enabled you'd click the event and get taken to the rolled back instance page. This could be frustrating at best and misleading/confusing at worst. Happy to be overruled by a tie-breaking vote here. Maxim Khutornenko wrote: | but if rollbacks are enabled you'd click the event and get taken to the rolled back instance page. The instance page aggregates task history by instance ID, right? This is a perfect example when having instance history is actually quite useful when debugging a rollback. Exactly. There's no rolled back instance page. There's just the instance page that shows you everything that's happened recently for a particular instance id. So based on your example of a rollback, you'd see the Active task as the rolled back one, and under the list of completed tasks, you'd see the one that failed and (hopefully) more easily be able to determine what went wrong. - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96394 --- On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
Re: Review Request 37761: Add a link to the instance page from instance events on the update page.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/#review96479 --- Ship it! Ship It! - David McLaughlin On Aug. 25, 2015, 3:01 p.m., Joshua Cohen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37761/ --- (Updated Aug. 25, 2015, 3:01 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Bugs: AURORA-1331 https://issues.apache.org/jira/browse/AURORA-1331 Repository: aurora Description --- Add a link to the instance page from instance events on the update page. Diffs - src/main/resources/scheduler/assets/update.html 3750aab342e326fc34d254dbfce791da0ca05ec6 Diff: https://reviews.apache.org/r/37761/diff/ Testing --- See screenshot. File Attachments Look, the instance numbers are blue, because they're links! https://reviews.apache.org/media/uploaded/files/2015/08/25/b0dc6715-be1a-4a81-992f-caf2efd847a6__Screen_Shot_2015-08-25_at_9.59.50_AM.png Thanks, Joshua Cohen
