You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@fluo.apache.org by GitBox <gi...@apache.org> on 2020/03/04 23:19:18 UTC
[GitHub] [fluo] jkosh44 opened a new pull request #1088: Use LeaderLatch to
determine if oracle exists
jkosh44 opened a new pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088
The old implementation of oracleExists checks to see if there are
more than 1 ZNodes under the ZookeeperPath.ORACLE_SERVER path. This
relies on the LeaderLatch implementation and uses a path given to
a Curator recipe, which is advised against by the Curator docs.
See https://cwiki.apache.org/confluence/display/CURATOR/TN7.
The new implementation utilizes LeaderLatch to count the number
of participants to see if there's more than 0.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] keith-turner commented on a change in pull request #1088:
Use LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389124612
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
Review comment:
Does it need to be public?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 commented on a change in pull request #1088: Use
LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389220643
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
+ int numOracles = 0;
+ try {
+ if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
Review comment:
@keith-turner just opened #1089
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] keith-turner commented on a change in pull request #1088:
Use LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389125330
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
+ int numOracles = 0;
+ try {
+ if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
Review comment:
It would be good to open an issue for numWorkers
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 commented on a change in pull request #1088: Use
LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r387993978
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
Review comment:
I added the static method back in so that it could be called by the static `oracleExists`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] keith-turner commented on a change in pull request #1088:
Use LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389124057
##########
File path: modules/integration-tests/src/main/java/org/apache/fluo/integration/client/FluoAdminImplIT.java
##########
@@ -286,4 +289,29 @@ public void testNumOracles() throws Exception {
Assert.assertEquals(0, admin.numOracles());
}
}
+
+ @Test
+ public void testNumOraclesWithMissingOraclePath() throws Exception {
+ oserver.stop();
+ try (CuratorFramework curator = CuratorUtil.newAppCurator(config);
+ FluoAdminImpl admin = new FluoAdminImpl(config)) {
+ curator.start();
+ oserver.awaitLeaderElection(3, TimeUnit.SECONDS);
+ curator.delete().forPath(ZookeeperPath.ORACLE_SERVER);
+
+ assertEquals(0, admin.numOracles());
Review comment:
Nice test.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] keith-turner commented on a change in pull request #1088:
Use LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389190807
##########
File path: modules/mapreduce/src/main/java/org/apache/fluo/mapreduce/FluoOutputFormat.java
##########
@@ -43,8 +43,7 @@
private static String PROPS_CONF_KEY = FluoOutputFormat.class.getName() + ".props";
@Override
- public void checkOutputSpecs(JobContext arg0) throws IOException, InterruptedException {
- }
+ public void checkOutputSpecs(JobContext arg0) throws IOException, InterruptedException {}
Review comment:
Maybe there was a previous commit where this was not properly formatted.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 commented on a change in pull request #1088: Use
LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r387993589
##########
File path: modules/mapreduce/src/main/java/org/apache/fluo/mapreduce/FluoOutputFormat.java
##########
@@ -43,8 +43,7 @@
private static String PROPS_CONF_KEY = FluoOutputFormat.class.getName() + ".props";
@Override
- public void checkOutputSpecs(JobContext arg0) throws IOException, InterruptedException {
- }
+ public void checkOutputSpecs(JobContext arg0) throws IOException, InterruptedException {}
Review comment:
This kept getting auto formatted by maven.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 commented on a change in pull request #1088: Use
LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389220671
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
Review comment:
Made it private in most recent commit
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 commented on a change in pull request #1088: Use
LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r388645454
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
+ int numOracles = 0;
+ try {
+ if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
Review comment:
So I removed the check for existence and then added a test that specifically tests the scenario when the oracle ZNode has been deleted. The test seems to work fine and `numOracles` returns 0 when the node doesn't exist.
It should be noted though that `numWorkers` follows a similar pattern
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 commented on a change in pull request #1088: Use
LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r388645454
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
+ int numOracles = 0;
+ try {
+ if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
Review comment:
So I removed the check for existence and then added a test that specifically tests the scenario when the oracle ZNode has been deleted. The test seems to work fine and `numOracles` returns 0 when the node doesn't exist.
It should be noted though that numWorkers follows a similar pattern
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] keith-turner commented on a change in pull request #1088:
Use LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r389191104
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
+ int numOracles = 0;
+ try {
+ if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
Review comment:
@jkosh44 I would be happy to open an issue for numWorkers unless you are interested in doing that
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] keith-turner commented on a change in pull request #1088:
Use LeaderLatch to determine if oracle exists
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#discussion_r388578931
##########
File path: modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
##########
@@ -505,30 +505,28 @@ private String getJarsFromClasspath() {
}
public static boolean oracleExists(CuratorFramework curator) {
- try {
- return curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null
- && !curator.getChildren().forPath(ZookeeperPath.ORACLE_SERVER).isEmpty();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
+ return numOracles(curator) > 0;
}
public boolean oracleExists() {
return oracleExists(getAppCurator());
}
- public int numOracles() {
- CuratorFramework curator = getAppCurator();
- if (oracleExists(curator)) {
- try {
+ public static int numOracles(CuratorFramework curator) {
+ int numOracles = 0;
+ try {
+ if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
Review comment:
The pattern of checking for existence before the operation has a race condition. Its possible that the node is deleted after the existence check but before the operation.
I took a look at the code and its possible this existence check may not be needed. The following code for `getParticipants()` calls `getSortedChildren()`
https://github.com/apache/curator/blob/959c1ca34f6ebcb11370180dfafbd8e85320dcd2/curator-recipes/src/main/java/org/apache/curator/framework/recipes/leader/LeaderLatch.java#L448
and `getSortedChildren()` seems like it may return empty list when the node does not exists.
https://github.com/apache/curator/blob/959c1ca34f6ebcb11370180dfafbd8e85320dcd2/curator-recipes/src/main/java/org/apache/curator/framework/recipes/locks/LockInternals.java#L172
Not sure if the version of Curator used by Fluo does this.
When dealing with ZK the pattern of trying the operation and catching NoNodeException is better than checking for existence before the operation because it avoids race conditions.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] kpm1985 commented on issue #1088: Use LeaderLatch to
determine if oracle exists
Posted by GitBox <gi...@apache.org>.
kpm1985 commented on issue #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088#issuecomment-596143707
Hello,
Ive been out for a while with health issues sapping energy but saw this.
Keith should remember a very similar race condition we tracked to Curator.
When we changed the recipe it fixed the race condition. It should be
documented on GitHub.
On Fri, Mar 6, 2020, 7:01 PM Keith Turner <no...@github.com> wrote:
> *@keith-turner* commented on this pull request.
> ------------------------------
>
> In
> modules/core/src/main/java/org/apache/fluo/core/client/FluoAdminImpl.java
> <https://github.com/apache/fluo/pull/1088#discussion_r389191104>:
>
> > }
>
> public boolean oracleExists() {
> return oracleExists(getAppCurator());
> }
>
> - public int numOracles() {
> - CuratorFramework curator = getAppCurator();
> - if (oracleExists(curator)) {
> - try {
> + public static int numOracles(CuratorFramework curator) {
> + int numOracles = 0;
> + try {
> + if (curator.checkExists().forPath(ZookeeperPath.ORACLE_SERVER) != null) {
>
> @jkosh44 <https://github.com/jkosh44> I would be happy to open an issue
> for numWorkers unless you are interested in doing that
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/fluo/pull/1088?email_source=notifications&email_token=AHED4LFZ7JFLRWDNPMEE2J3RGGA2VA5CNFSM4LB5OYW2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCYMDF4Q#discussion_r389191104>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHED4LGI2RPTAHTAP5UVDFTRGGA2VANCNFSM4LB5OYWQ>
> .
>
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [fluo] jkosh44 merged pull request #1088: Use LeaderLatch to
determine if oracle exists
Posted by GitBox <gi...@apache.org>.
jkosh44 merged pull request #1088: Use LeaderLatch to determine if oracle exists
URL: https://github.com/apache/fluo/pull/1088
----------------------------------------------------------------
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
With regards,
Apache Git Services