You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/02/22 13:24:10 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1943: Improve tablet details in Monitor. Closes #1940
milleruntime opened a new pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r581948719
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,8 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet
+ <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png"></th>
Review comment:
> The non-breaking space is moot if it is followed by a breaking space (newline, indentation).
I thought the template will ignore whitespace/format when generating the page (hence our use of the ` ` encodings)?
> I'm more concerned, however, that the new image file added to the PR seems to be showing up as an "empty file". Not sure what's going on with that. Bug in GitHub? Did you add an empty file? There might be an existing bootstrap icon that can be used, that is already in the monitor's resources.
The file I committed is the image and not empty so I don't know what is going on with github.
<pre>
08:13:02 (tablet-details-monitor) ~/workspace/accumulo$ file server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/images/circled-information-icon.png
server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/images/circled-information-icon.png: PNG image data, 128 x 128, 8-bit/color RGBA, non-interlaced
08:13:07 (tablet-details-monitor) ~/workspace/accumulo$ du -sh server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/images/circled-information-icon.png
16K server/monitor/src/main/resources/org/apache/accumulo/monitor/resources/images/circled-information-icon.png
</pre>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r580577471
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,7 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th>Tablet (run shell cmd: getsplits -v)</th>
Review comment:
I responded on #1940 but the short answer is no, it wouldn't make sense.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r580427487
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,7 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th>Tablet (run shell cmd: getsplits -v)</th>
Review comment:
From my comments on issue #1940 - would it be more useful if the column displayed the table-id and assigned server? Otherwise I agree with the tool-tip suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r582854432
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,8 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet
+ <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png"></th>
Review comment:
That is a possibility. I would have to track down where I got it from. I will try the bootstrap icon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r581363864
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,8 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet
+ <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png"></th>
Review comment:
Maybe `.glyphicon-info-sign` will work?
```suggestion
<th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet <span class="glyphicon glyphicon-info-sign" aria-hidden="true"></span> </th>
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r580371041
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,7 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th>Tablet (run shell cmd: getsplits -v)</th>
Review comment:
I like the information symbol idea. I will look and see if our web deps already have an icon included. The unicode you used isn't showing up for me
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r580351370
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,7 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th>Tablet (run shell cmd: getsplits -v)</th>
Review comment:
I had thought of that but didn't think that it was obvious enough. Also, none of the columns on that table currently have tooltips.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r581360373
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,8 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet
+ <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png"></th>
Review comment:
The non-breaking space is moot if it is followed by a breaking space (newline, indentation).
Also, the image tag isn't proper XML. I know HTML5 is pretty forgiving, but it's nice to have the tags be balanced with either a closing tag element or the terminal `/>` at the end of the start tag.
```suggestion
<th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png" /></th>
```
I'm more concerned, however, that the new image file added to the PR seems to be showing up as an "empty file". Not sure what's going on with that. Bug in GitHub? Did you add an empty file? There might be an existing bootstrap icon that can be used, that is already in the monitor's resources.
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,8 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet
+ <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png"></th>
Review comment:
The non-breaking space is moot if it is followed by a breaking space (newline, indentation).
Also, the image tag isn't proper XML. I know HTML5 is pretty forgiving, but it's nice to have the tags be balanced with either a closing tag element or the terminal `/>` at the end of the start tag.
```suggestion
<th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png" /> </th>
```
I'm more concerned, however, that the new image file added to the PR seems to be showing up as an "empty file". Not sure what's going on with that. Bug in GitHub? Did you add an empty file? There might be an existing bootstrap icon that can be used, that is already in the monitor's resources.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r580342997
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,7 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th>Tablet (run shell cmd: getsplits -v)</th>
Review comment:
This should be in the mouseover/tooltip, instead of in the column.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r580367900
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,7 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th>Tablet (run shell cmd: getsplits -v)</th>
Review comment:
I just think that leaving it in the column header makes the interface messy, since it's instructions, and not part of the actual name/description of the field/column.
It's also a bit weird to specifically call out a separate tool without a more complete sentence, which we could do in a tooltip. The monitor and shell are largely independent tools that users could mix-and-match and replace with other tools, and different users might interface with each of them. So the monitor's explicit assumption that the user viewing the monitor has access to the Accumulo-provided shell and that they will understand "run shell cmd" as short for "run the Accumulo Shell client utility provided in the default Accumulo binary distribution tarball" are a little bit of a stretch for me.
Maybe the availability of a tooltip could be made more obvious with a little information symbol 🛈 or similar. I was thinking a longer tip, like `To associate the tablets listed here with their entries in the metadata table, use the <code>getsplits -v</code> command in the Accumulo Shell utility.`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1943: Improve tablet details in Monitor. Closes #1940
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1943:
URL: https://github.com/apache/accumulo/pull/1943#discussion_r582091845
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/server.ftl
##########
@@ -141,7 +141,8 @@
<thead>
<tr>
<th>Table </th>
- <th>Tablet </th>
+ <th title="Run 'getsplits -v' in the Accumulo Shell to associate the encoded tablets with their actual splits.">Tablet
+ <img height="10em" width="10em" alt="information" src="/resources/images/circled-information-icon.png"></th>
Review comment:
> > The non-breaking space is moot if it is followed by a breaking space (newline, indentation).
>
> I thought the template will ignore whitespace/format when generating the page (hence our use of the ` ` encodings)?
>
The non-breaking space is there because datatables appends the sort icon to the right of it, and we want to make sure there's a gap between the title and that icon that it adds, but we also want to make sure that the icon doesn't wrap to the next line when the screen is resized. Hence a non-breaking space instead of a regular space for that gap. In HTML, any amount of non-breaking space between two elements permits a line break to be inserted. So, if we want to avoid a line break, elements must be immediately adjacent. In this case, datatables should append its sorting icon to be immediately adjacent to the non-breaking space, and we should have only non-breaking spaces in our own title.
> The file I committed is the image and not empty so I don't know what is going on with github.
Not sure why GitHub says it's empty. Where did the icon come from? Any copyright issues with it? I think the bootstrap built-in is best, if we can get it working, rather than try to track copyright license issues from an additional source.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org