You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by joshelser <gi...@git.apache.org> on 2016/09/09 23:09:25 UTC
[GitHub] accumulo pull request #150: ACCUMULO-4424 Start thrift servers immediately o...
GitHub user joshelser opened a pull request:
https://github.com/apache/accumulo/pull/150
ACCUMULO-4424 Start thrift servers immediately on ha components
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/joshelser/accumulo start-thrift-servers-immediately-on-ha-components
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/accumulo/pull/150.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #150
----
commit c75660d6d2fc918f421124ca189103dbc3613625
Author: Josh Elser <el...@apache.org>
Date: 2016-09-09T20:27:13Z
ACCUMULO-4424 First pass fix for starting thrift servers on master/monitor immediately
commit 1b7f4eb03672afc62a81b437a0e6c80bc2d3f3e4
Author: Josh Elser <el...@apache.org>
Date: 2016-09-09T20:54:53Z
Shut up checkstyle/formatter
commit b3a03af9d23755735d11023685eb787641167b41
Author: Josh Elser <el...@apache.org>
Date: 2016-09-09T22:41:42Z
Forgot to implement the isActiveService() method for the Monitor
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #150: ACCUMULO-4424 Start thrift servers immediately on ha co...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on the issue:
https://github.com/apache/accumulo/pull/150
+1 to the strategy. Haven't checked out the code to test. The test case looks good, though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #150: ACCUMULO-4424 Start thrift servers immediately on ha co...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/150
@ctubbsii @billierinaldi might be interested in this one. I need to write some tests, but I verified that things are working as expected in a local installation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #150: ACCUMULO-4424 Start thrift servers immediately o...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/accumulo/pull/150
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #150: ACCUMULO-4424 Start thrift servers immediately o...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/150#discussion_r78467243
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -51,8 +52,18 @@
abstract protected String getTitle(HttpServletRequest req);
+ public void checkIfActive() throws IOException {
+ // If the HighlyAvailableService is not initialized or it's not the active service, throw an exception
+ // to prevent processing of the servlet.
+ if (null == Monitor.HA_SERVICE_INSTANCE || !Monitor.HA_SERVICE_INSTANCE.isActiveService()) {
+ throw new IOException("This is not the active Monitor", new NotActiveServiceException());
--- End diff --
[`HttpServletResponse`][1] has several useful methods for sending redirects, errors, or otherwise manipulating the status.
[1]: http://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletResponse.html
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #150: ACCUMULO-4424 Start thrift servers immediately on ha co...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/150
b36d9a8 sends HTTP/503 with the error message instead of propagating the exception.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #150: ACCUMULO-4424 Start thrift servers immediately o...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/150#discussion_r78467505
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -51,8 +52,18 @@
abstract protected String getTitle(HttpServletRequest req);
+ public void checkIfActive() throws IOException {
+ // If the HighlyAvailableService is not initialized or it's not the active service, throw an exception
+ // to prevent processing of the servlet.
+ if (null == Monitor.HA_SERVICE_INSTANCE || !Monitor.HA_SERVICE_INSTANCE.isActiveService()) {
+ throw new IOException("This is not the active Monitor", new NotActiveServiceException());
--- End diff --
Oh duh. I forgot about that. Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #150: ACCUMULO-4424 Start thrift servers immediately o...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/150#discussion_r78462981
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -51,8 +52,18 @@
abstract protected String getTitle(HttpServletRequest req);
+ public void checkIfActive() throws IOException {
+ // If the HighlyAvailableService is not initialized or it's not the active service, throw an exception
+ // to prevent processing of the servlet.
+ if (null == Monitor.HA_SERVICE_INSTANCE || !Monitor.HA_SERVICE_INSTANCE.isActiveService()) {
+ throw new IOException("This is not the active Monitor", new NotActiveServiceException());
--- End diff --
It'd probably be better to return an explicit 503 on the response, rather than throw an IOException here.
We could also try to discern the active monitor (if there is one), and return a 307 redirect.
Or, we could have partial functionality (personally, I'm not a fan of the monitor using this locking mechanism... I'd rather support running multiple monitors).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #150: ACCUMULO-4424 Start thrift servers immediately o...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/150#discussion_r78464250
--- Diff: server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---
@@ -51,8 +52,18 @@
abstract protected String getTitle(HttpServletRequest req);
+ public void checkIfActive() throws IOException {
+ // If the HighlyAvailableService is not initialized or it's not the active service, throw an exception
+ // to prevent processing of the servlet.
+ if (null == Monitor.HA_SERVICE_INSTANCE || !Monitor.HA_SERVICE_INSTANCE.isActiveService()) {
+ throw new IOException("This is not the active Monitor", new NotActiveServiceException());
--- End diff --
Can you recommend how I might go about this? I'm not sure how to propagate that information back up from a servlet.
> We could also try to discern the active monitor (if there is one), and return a 307 redirect.
Sure, this would be a great addition.
> Or, we could have partial functionality (personally, I'm not a fan of the monitor using this locking mechanism... I'd rather support running multiple monitors).
Wouldn't be too bad, we would just have to make the master capable of sending to multiple monitors. But, again, out of scope for this changeset.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #150: ACCUMULO-4424 Start thrift servers immediately on ha co...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/150
Just pushed an IT for this change in 2f060a4.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---