You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by blrunner <gi...@git.apache.org> on 2014/07/15 18:55:51 UTC

[GitHub] tajo pull request: TAJO-704: TajoMaster HA

GitHub user blrunner opened a pull request:

    https://github.com/apache/tajo/pull/77

    TAJO-704: TajoMaster HA

    I implemented TajoMaster HA Manager utilizing HDFS Cluster. I defined a interface named HAManager, and I implemented HAManagerWithHDFS with the interface. 
    
    To use TajoMaster HA, you have to set tajo.master.ha.enable=true at tajo-site.xml. And then, you just run standby master. All master informations will record to hdfs cluster, and all tajo service will refer master service address using hdfs cluster.
    
    For reference, this patch includes a few issues as follows:
    
    - It just implemented one unit test case because of TAJO-942. 
    - If you kill current master and you already ran tsql, you have to restart tsql. because new master doesn't know existing session ids. We need to handle this issue at another ticket.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/blrunner/tajo TAJO-704

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/tajo/pull/77.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 #77
    
----
commit 47d6c6a32010621b94b31a9871d3a5cd44968667
Author: blrunner <jh...@gruter.com>
Date:   2014-07-15T14:32:32Z

    initial commit: TajoMaster HA Manager utilizing HDFS Cluster.

commit 0b6276738ad4be278d9ab7d222fef78419d42b62
Author: blrunner <jh...@gruter.com>
Date:   2014-07-15T16:25:34Z

    Adding HA elector to client side.

----


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16218468
  
    --- Diff: tajo-client/src/main/java/org/apache/tajo/client/TajoAdmin.java ---
    @@ -416,12 +420,41 @@ public void processKill(Writer writer, String queryIdStr)
     
       private void processMasters(Writer writer) throws ParseException, IOException,
           ServiceException, SQLException {
    -    String confMasterServiceAddr = tajoClient.getConf().getVar(TajoConf.ConfVars.TAJO_MASTER_UMBILICAL_RPC_ADDRESS);
    -    InetSocketAddress masterAddress = NetUtils.createSocketAddr(confMasterServiceAddr);
    -    writer.write(masterAddress.getHostName());
    -    writer.write("\n");
    +    checkMasterStatus();
    +    if (tajoConf.getBoolVar(TajoConf.ConfVars.TAJO_MASTER_HA_ENABLE)) {
    +
    +      List<String> list = HAServiceUtil.getMasters(tajoConf);
    +      int i = 0;
    +      for (String master : list) {
    +        if (i > 0) {
    +          writer.write(" ");
    +        }
    +        writer.write(master);
    +        i++;
    +      }
    +      writer.write("\n");
    +    } else {
    +      String confMasterServiceAddr = tajoClient.getConf().getVar(TajoConf.ConfVars.TAJO_MASTER_UMBILICAL_RPC_ADDRESS);
    +      InetSocketAddress masterAddress = NetUtils.createSocketAddr(confMasterServiceAddr);
    +      writer.write(masterAddress.getHostName());
    +      writer.write("\n");
    +    }
       }
     
    +  // In TajoMaster HA mode, if TajoAdmin can't connect existing active master,
    +  // this should try to connect new active master.
    +  private void checkMasterStatus() {
    --- End diff --
    
    TajoAdmin.checkMasterStatus() function has the same name and shares some codes with TajoCli.checkMasterStatus(). We can move these functions to one location such as HAServiceUtil, because it will help the code maintainance.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16218363
  
    --- Diff: tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java ---
    @@ -628,6 +633,31 @@ public void close() {
         }
       }
     
    +  // In TajoMaster HA mode, if TajoCli can't connect existing active master,
    +  // this should try to connect new active master.
    +  private void checkMasterStatus() {
    +    try {
    +      if (conf.getBoolVar(TajoConf.ConfVars.TAJO_MASTER_HA_ENABLE)) {
    +        if (!HAServiceUtil.isMasterAlive(conf.get(TajoConf.ConfVars.TAJO_MASTER_CLIENT_RPC_ADDRESS
    +            .varname), conf)) {
    +          String baseDatabase = client.getBaseDatabase();
    +          conf.set(ConfVars.TAJO_MASTER_CLIENT_RPC_ADDRESS.varname,
    +              HAServiceUtil.getMasterClientName(conf));
    +
    +          client.close();
    +          commands.clear();
    +
    +          client = new TajoClient(conf, baseDatabase);
    +          context.setCurrentDatabase(client.getCurrentDatabase());
    +
    +          initHistory();
    +          initCommands();
    +        }
    +      }
    +    } catch (Exception e) {
    --- End diff --
    
    I think that exceptions should be handled properly. It seems that the IOException can occur when a new tajo client is created. If an IOException occurs, the 'client' value will be not updated, and TajoCli might try to connect to the old master.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51086877
  
    I implemented the HA admin tool, you can use it as follows:
    - $TAJO_HOME/bin/tajo haadmin -formatHA
    - $TAJO_HOME/bin/tajo haadmin -transitionToActive <target tajo.master.umbilical-rpc.address>
    - $TAJO_HOME/bin/tajo haadmin -transitionToBackup <target tajo.master.umbilical-rpc.address>
    - $TAJO_HOME/bin/tajo haadmin -getState <target tajo.master.umbilical-rpc.address>
    
    * Note
    If you want to format HA state, you have to shutdown your tajo cluster before formatting.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-52645251
  
    Hi @jihoonson 
    
    Thank you for your review.
    I've just committed it to the master branch.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner closed the pull request at:

    https://github.com/apache/tajo/pull/77


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51139716
  
    Thanks for your understanding. :)


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-50131715
  
    Hi, @jihoonson 
    
    Thanks for your detailed review. I replied for your descriptions as follows:
    
    * I've just fixed NPE in the web UI via Hyunsik. Because my desktop has broken down.
    
    * DerbyStore is not appropriate for TajoMaster HA. As you know, just one tajo master invoke the store. If another master try to invoke it, it never run itself. Also, if we set another catalog uri for backup master, it is a incorrect configuration. Because they are unequal in every way.
    
    * If you run two masters on same host and you set same directory for pid, you must meet pid error. But I think that it is trivial problem. Because we can avoid easily by documents. 
    
    * If backup master become active, existing running query will finish successfully, and you will see success message in tsql. But you will never find executed query history in the web UI. Because TajoMaster contains its query history in memory, thus backup master can't know previous active master information. I already know this problem. And I'll save query simple history in share storage (like HDFS). 
    
     For reference, I'll write documents for TajoMaster HA. And I'll update start-tajo.sh to start backup masters. 
    
    Cheers


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-52621086
  
    +1
    Thanks for your work!!


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16218142
  
    --- Diff: tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java ---
    @@ -628,6 +633,31 @@ public void close() {
         }
       }
     
    +  // In TajoMaster HA mode, if TajoCli can't connect existing active master,
    +  // this should try to connect new active master.
    +  private void checkMasterStatus() {
    +    try {
    +      if (conf.getBoolVar(TajoConf.ConfVars.TAJO_MASTER_HA_ENABLE)) {
    +        if (!HAServiceUtil.isMasterAlive(conf.get(TajoConf.ConfVars.TAJO_MASTER_CLIENT_RPC_ADDRESS
    +            .varname), conf)) {
    +          String baseDatabase = client.getBaseDatabase();
    +          conf.set(ConfVars.TAJO_MASTER_CLIENT_RPC_ADDRESS.varname,
    --- End diff --
    
    It would be better to use conf.setVar() instead of conf.set().


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16219222
  
    --- Diff: tajo-core/src/main/java/org/apache/tajo/webapp/QueryExecutorServlet.java ---
    @@ -271,6 +272,17 @@ public void setStop() {
         public void run() {
           startTime = System.currentTimeMillis();
           try {
    +        // In TajoMaster HA mode, if this servlet can't connect existing active master,
    +        // this should try to connect new active master.
    +        TajoConf conf = tajoClient.getConf();
    --- End diff --
    
    This is also same with TajoAdmin.checkMasterStatus(). If you move checkMasterStatus() to any other util classes, please update this code, too.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51930714
  
    Hi @jihoonson 
    
    I would appreciate it if you could review the latest patch.
    
    Cheers


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-49288171
  
    I've rebased the patch against master branch.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16218650
  
    --- Diff: tajo-common/src/main/java/org/apache/tajo/util/HAServiceUtil.java ---
    @@ -0,0 +1,297 @@
    +/**
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.tajo.util;
    +
    +import org.apache.hadoop.fs.*;
    +import org.apache.tajo.TajoConstants;
    +import org.apache.tajo.conf.TajoConf;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.net.InetSocketAddress;
    +import java.net.Socket;
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +public class HAServiceUtil {
    +
    +  private final static int MASTER_UMBILICAL_RPC_ADDRESS = 1;
    +  private final static int MASTER_CLIENT_RPC_ADDRESS = 2;
    +  private final static int RESOURCE_TRACKER_RPC_ADDRESS = 3;
    +  private final static int CATALOG_ADDRESS = 4;
    +  private final static int MASTER_INFO_ADDRESS = 5;
    +
    +  public static InetSocketAddress getMasterUmbilicalAddress(TajoConf conf) {
    +    return getMasterAddress(conf, MASTER_UMBILICAL_RPC_ADDRESS);
    +  }
    +
    +  public static String getMasterUmbilicalName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getMasterUmbilicalAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getMasterClientAddress(TajoConf conf) {
    +    return getMasterAddress(conf, MASTER_CLIENT_RPC_ADDRESS);
    +  }
    +
    +  public static String getMasterClientName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getMasterClientAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getResourceTrackerAddress(TajoConf conf) {
    +    return getMasterAddress(conf, RESOURCE_TRACKER_RPC_ADDRESS);
    +  }
    +
    +  public static String getResourceTrackerName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getResourceTrackerAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getCatalogAddress(TajoConf conf) {
    +    return getMasterAddress(conf, CATALOG_ADDRESS);
    +  }
    +
    +  public static String getCatalogName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getCatalogAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getMasterInfoAddress(TajoConf conf) {
    +    return getMasterAddress(conf, MASTER_INFO_ADDRESS);
    +  }
    +
    +  public static String getMasterInfoName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getMasterInfoAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getMasterAddress(TajoConf conf, int type) {
    +    InetSocketAddress masterAddress = null;
    +
    +    if (conf.getBoolVar(TajoConf.ConfVars.TAJO_MASTER_HA_ENABLE)) {
    +      try {
    +        FileSystem fs = getFileSystem(conf);
    +        Path[] paths = getSystemPath(conf);
    +
    +        if (fs.exists(paths[1])) {
    +          FileStatus[] files = fs.listStatus(paths[1]);
    +
    +          if (files.length == 1) {
    +            Path file = files[0].getPath();
    +            String hostAddress = file.getName().replaceAll("_", ":");
    +            FSDataInputStream stream = fs.open(file);
    +            String data = stream.readUTF();
    +            stream.close();
    +
    +            String[] addresses = data.split("_");
    +
    +            switch (type) {
    +              case 1:
    --- End diff --
    
    It would be great if you use the defined name instead of defined values. 


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16218653
  
    --- Diff: tajo-common/src/main/java/org/apache/tajo/util/HAServiceUtil.java ---
    @@ -0,0 +1,297 @@
    +/**
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +package org.apache.tajo.util;
    +
    +import org.apache.hadoop.fs.*;
    +import org.apache.tajo.TajoConstants;
    +import org.apache.tajo.conf.TajoConf;
    +
    +import javax.net.SocketFactory;
    +import java.io.IOException;
    +import java.net.InetSocketAddress;
    +import java.net.Socket;
    +import java.util.ArrayList;
    +import java.util.List;
    +
    +public class HAServiceUtil {
    +
    +  private final static int MASTER_UMBILICAL_RPC_ADDRESS = 1;
    +  private final static int MASTER_CLIENT_RPC_ADDRESS = 2;
    +  private final static int RESOURCE_TRACKER_RPC_ADDRESS = 3;
    +  private final static int CATALOG_ADDRESS = 4;
    +  private final static int MASTER_INFO_ADDRESS = 5;
    +
    +  public static InetSocketAddress getMasterUmbilicalAddress(TajoConf conf) {
    +    return getMasterAddress(conf, MASTER_UMBILICAL_RPC_ADDRESS);
    +  }
    +
    +  public static String getMasterUmbilicalName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getMasterUmbilicalAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getMasterClientAddress(TajoConf conf) {
    +    return getMasterAddress(conf, MASTER_CLIENT_RPC_ADDRESS);
    +  }
    +
    +  public static String getMasterClientName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getMasterClientAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getResourceTrackerAddress(TajoConf conf) {
    +    return getMasterAddress(conf, RESOURCE_TRACKER_RPC_ADDRESS);
    +  }
    +
    +  public static String getResourceTrackerName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getResourceTrackerAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getCatalogAddress(TajoConf conf) {
    +    return getMasterAddress(conf, CATALOG_ADDRESS);
    +  }
    +
    +  public static String getCatalogName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getCatalogAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getMasterInfoAddress(TajoConf conf) {
    +    return getMasterAddress(conf, MASTER_INFO_ADDRESS);
    +  }
    +
    +  public static String getMasterInfoName(TajoConf conf) {
    +    return NetUtils.normalizeInetSocketAddress(getMasterInfoAddress(conf));
    +  }
    +
    +  public static InetSocketAddress getMasterAddress(TajoConf conf, int type) {
    +    InetSocketAddress masterAddress = null;
    +
    +    if (conf.getBoolVar(TajoConf.ConfVars.TAJO_MASTER_HA_ENABLE)) {
    +      try {
    +        FileSystem fs = getFileSystem(conf);
    +        Path[] paths = getSystemPath(conf);
    +
    +        if (fs.exists(paths[1])) {
    +          FileStatus[] files = fs.listStatus(paths[1]);
    +
    +          if (files.length == 1) {
    +            Path file = files[0].getPath();
    +            String hostAddress = file.getName().replaceAll("_", ":");
    +            FSDataInputStream stream = fs.open(file);
    +            String data = stream.readUTF();
    +            stream.close();
    +
    +            String[] addresses = data.split("_");
    +
    +            switch (type) {
    +              case 1:
    +                masterAddress = NetUtils.createSocketAddr(hostAddress);
    +                break;
    +              case 2:
    +                masterAddress = NetUtils.createSocketAddr(addresses[0]);
    +                break;
    +              case 3:
    +                masterAddress = NetUtils.createSocketAddr(addresses[1]);
    +                break;
    +              case 4:
    +                masterAddress = NetUtils.createSocketAddr(addresses[2]);
    +                break;
    +              case 5:
    +                masterAddress = NetUtils.createSocketAddr(addresses[3]);
    +                break;
    +              default:
    +                break;
    +            }
    +          }
    +        }
    +
    +      } catch (Exception e) {
    +        e.printStackTrace();
    +      }
    +    }
    +
    +    if (masterAddress == null) {
    +      switch (type) {
    +        case 1:
    --- End diff --
    
    It would be great if you use the defined name instead of defined values. 


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-50111555
  
    @blrunner, thanks for the great contribution!
    I tested the latest patch on a real cluster, and it seems that there are a couple of problems.
    Here are descriptions.
    * When HA mode is disabled, the web UI doesn't work well.
    * There are some problems when derby is used as catalog store. 
     * I couldn't execute the backup master due to the duplicated pid problem. 
     * The duplicated catalog uri also incurred a problem.
     * When I used a different catalog uri as a workaround for the above problem, there were differences between the catalog of active master and that of backup master, obviously. 
    * I manually added tables to the backup master catalog to test the failover during the query execution. When the active master is killed, the backup master becomes to active and every worker is registered to the changed active master successfully, but the information of executed query is disappeared.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-50256948
  
    @blrunner, thanks for your kind reply and documentation!
    I'll further review your patch.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-49950851
  
    Thanks @blrunner,
    I'll review today.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-52144626
  
    @blrunner, thanks for your great work!
    The patch looks good to me in overall. I left some minor comments.
    I'll conduct some tests on a real cluster.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51987735
  
    Sorry for late review.
    I'll review it today.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-52456134
  
    Hi @jihoonson.
    
    I updated the patch with your ideas. For reference, I implemented another utility class for client. Because I couldn't implement in tajo common module by dependency issue. 
    
    I found some client side bugs when I had tested the patch on testing cluster (consists of 10 nodes). So, I fixed it and committed it to my private branch.
    
    If you can review it again, I will be really glad for you. :)


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51136842
  
    Hi @jihoonson,
    
    No problem and have a good holidays. :)


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51118478
  
    Thanks @blrunner and sorry but I am hard to review now because I'm in short holidays from today. I'll come back after two days. 
    May I review after holydays?


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-51043233
  
    Hi @jihoonson 
    
    I added a document for TajoMaster HA.
    And I'll implement shell script to launch masters after committing https://issues.apache.org/jira/browse/TAJO-990.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by blrunner <gi...@git.apache.org>.
Github user blrunner commented on the pull request:

    https://github.com/apache/tajo/pull/77#issuecomment-49386901
  
    I added auto failover for running queries.


---
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] tajo pull request: TAJO-704: TajoMaster HA

Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on a diff in the pull request:

    https://github.com/apache/tajo/pull/77#discussion_r16218127
  
    --- Diff: tajo-client/src/main/java/org/apache/tajo/cli/TajoCli.java ---
    @@ -628,6 +633,31 @@ public void close() {
         }
       }
     
    +  // In TajoMaster HA mode, if TajoCli can't connect existing active master,
    +  // this should try to connect new active master.
    +  private void checkMasterStatus() {
    +    try {
    +      if (conf.getBoolVar(TajoConf.ConfVars.TAJO_MASTER_HA_ENABLE)) {
    +        if (!HAServiceUtil.isMasterAlive(conf.get(TajoConf.ConfVars.TAJO_MASTER_CLIENT_RPC_ADDRESS
    --- End diff --
    
    It would be better to use conf.getVar() instead of conf.get().


---
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.
---