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/04/14 16:13:21 UTC
[GitHub] [accumulo] karthick-rn opened a new pull request #2020: Remove ZK stats from Monitor overview page
karthick-rn opened a new pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020
This PR removes ZK stats table from the "Accumulo Overview" page and includes ZK host:port details in the "About" dialog box as shown below. Addresses #1645.
About:
--
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 #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#discussion_r613954945
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/overview.ftl
##########
@@ -24,7 +24,7 @@
</div>
</div>
<div class="row">
- <div class="col-sm-6" id="manager">
+ <div class="col-sm-6 col-sm-offset-3" id="manager">
Review comment:
I was thinking something like these boxes in this bootstrap example: https://getbootstrap.com/docs/4.0/examples/pricing/
But, I would defer to somebody more experienced with site design. This comment was mostly just a passing thought about how we could probably get rid of the table format for the information at the top of the overview page.
--
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 #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#discussion_r613431309
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/overview.ftl
##########
@@ -24,7 +24,7 @@
</div>
</div>
<div class="row">
- <div class="col-sm-6" id="manager">
+ <div class="col-sm-6 col-sm-offset-3" id="manager">
Review comment:
I think the information in this table can probably be "prettified" a bit, by moving it out of the HTML table format, and into some other format for presentation, but centering it is an acceptable interim solution.
--
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 merged pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020
--
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 pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#issuecomment-821328744
I think this caused a failure in the WebViewsIT integration test. It's probably due to a test case against one of the removed REST endpoints that should also be removed, but I haven't investigated yet.
--
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] karthick-rn commented on pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
karthick-rn commented on pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#issuecomment-820347829
> The removal of the `refreshZKTable` function from `overview.js` and the place where that function is called can also be included in this PR, and any other related javascript code that kept this table up-to-date can also be removed (if any), as well as any related CSS stylesheet information that was specific to this table.
It was a bit of oversight from my side, thanks for mentioning the clean up activity. I have removed the `refreshZKTable` function, calls to the function & also the 3 java programs under `/monitor/rest/zk` directory in the new commit. Hopefully, these are the one's (I think) that had kept the ZK table up-to-date. Let me know if there is anything else?
I also did a regression test and ensured the behaviour is still the same.
--
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 pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#issuecomment-819676434
> Why? That information is useful in certain situations - especially if you have zk node failures or need to do a zk node restart.
The reasoning seems discussed on the referenced issue. The underlying issue is that our ability to display this is dependent on an insecure and outdated way of retrieving information from the ZK servers (4LW) that have been replaced by a new way (Adminserver) and is broken by default, without significant extra work to support retrieving the information from ZK under a variety of circumstances (such as when TLS is enabled for the Adminserver).
As hinted at on the issue, but perhaps not explicitly stated, it's better to remove the responsibility of displaying ZK stats from Accumulo and let users monitor their own ZK instances their own way, via features provided by ZK, rather than us. The Accumulo monitor should not be responsible for monitoring other services in a user's cluster, but should be narrowly scoped to monitor Accumulo itself, leaving the responsibility of monitoring other services in a user's cluster to those other projects or to users. Even if the information is useful, it's not reasonable to consider the monitoring of other services in a user's cluster to be in scope for Accumulo, especially as the amount of code required to support it grows (to handle 4LW and Adminserver, with and without TLS, etc.)
--
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 pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#issuecomment-822587653
> expect(contextMock.getZooKeepers()).andReturn("foo:2181").anyTimes();
Tested this change and it works. Applied in 8dfe443af55f5d3aacb12fb38c5002dcb61cb772
--
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 #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#discussion_r613427475
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/modals.ftl
##########
@@ -45,6 +45,10 @@
<div class="col-sm-4 text-right">Instance Id</div>
<div class="col-sm-6 text-left">${instance_id}</div>
</div>
+ <div class="row">
+ <div class="col-sm-4 text-right">Zookeeper Hosts</div>
Review comment:
Trying to keep the capitalization consistent with how the ZooKeeper project presents it on their website, the `K` should be uppercase.
```suggestion
<div class="col-sm-4 text-right">ZooKeeper Hosts</div>
```
--
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] karthick-rn commented on a change in pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
karthick-rn commented on a change in pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#discussion_r613936392
##########
File path: server/monitor/src/main/resources/org/apache/accumulo/monitor/templates/overview.ftl
##########
@@ -24,7 +24,7 @@
</div>
</div>
<div class="row">
- <div class="col-sm-6" id="manager">
+ <div class="col-sm-6 col-sm-offset-3" id="manager">
Review comment:
Hi @ctubbsii, If you have any thoughts/ideas on the format let me know I'm happy to work on it. Thanks
--
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] karthick-rn commented on pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
karthick-rn commented on pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#issuecomment-822422194
> I think this caused a failure in the WebViewsIT integration test. It's probably due to a test case against one of the removed REST endpoints that should also be removed, but I haven't investigated yet.
@ctubbsii To investigate this, I ran `mvn clean verify -Dit.test=WebViewsIT -Dtest=foo` which resulted in the below failure.
> [ERROR] Failures:
[ERROR] WebViewsIT.finishMocks:107 On mock #0 (zero indexed):
Unexpected method calls:
ServerContext.getZooKeepers()
[ERROR] WebViewsIT.testGetTablesConstraintPassing:143 should return status 200 expected:<200> but was:<500>
[INFO]
[ERROR] Tests run: 4, Failures: 2, Errors: 0, Skipped: 0
As part of the change, I included `getZooKeepers()` in `WebViews.java` which was missing in the relevant integration test.
After adding `getZooKeepers()` to `createMocks()` method (as highlighted below) the integration test ran successfully. Let me know if you're ok with this change? Thanks
<pre>
expect(contextMock.getInstanceID()).andReturn("foo").atLeastOnce();
expect(contextMock.getInstanceName()).andReturn("foo").anyTimes();
<b>expect(contextMock.getZooKeepers()).andReturn("foo:2181").anyTimes(); </b>
</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] EdColeman commented on pull request #2020: Remove ZK stats from Monitor overview page
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #2020:
URL: https://github.com/apache/accumulo/pull/2020#issuecomment-819645418
Why? That information is useful in certain situations - especially if you have zk node failures or need to do a zk node restart.
--
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