[GitHub] incubator-tephra pull request #10: TEPHRA-179 Transaction service high avail...

2016-09-12 Thread poornachandra 
Github user poornachandra commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/10#discussion_r78493478
  
--- Diff: 
tephra-core/src/main/java/org/apache/tephra/runtime/DefaultTransactionManagerProvider.java
 ---
@@ -0,0 +1,71 @@
+/*
+ * 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.tephra.runtime;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import com.google.inject.Provider;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.tephra.TransactionManager;
+import org.apache.twill.zookeeper.ZKClient;
+import org.apache.twill.zookeeper.ZKClientService;
+
+/**
+ * A provider for {@link TransactionManager} that provides a new instance 
every time.
+ */
+public class DefaultTransactionManagerProvider implements 
Provider {
+  private final Configuration conf;
+  private final ZKClientService zkClientService;
+
+  @Inject
+  public DefaultTransactionManagerProvider(Configuration conf, 
ZKClientService zkClientService) {
+this.conf = conf;
+this.zkClientService = zkClientService;
+  }
+
+  @Override
+  public TransactionManager get() {
+// Create a new injector every time since Guice services cannot be 
restarted TEPHRA-179
+Injector injector = Guice.createInjector(
--- End diff --

Filed JIRA https://issues.apache.org/jira/browse/TEPHRA-182 for follow-up


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #10: TEPHRA-179 Transaction service high avail...

2016-09-12 Thread chtyim 
Github user chtyim commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/10#discussion_r78407204
  
--- Diff: 
tephra-core/src/main/java/org/apache/tephra/runtime/DefaultTransactionManagerProvider.java
 ---
@@ -0,0 +1,71 @@
+/*
+ * 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.tephra.runtime;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import com.google.inject.Module;
+import com.google.inject.Provider;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.tephra.TransactionManager;
+import org.apache.twill.zookeeper.ZKClient;
+import org.apache.twill.zookeeper.ZKClientService;
+
+/**
+ * A provider for {@link TransactionManager} that provides a new instance 
every time.
+ */
+public class DefaultTransactionManagerProvider implements 
Provider {
+  private final Configuration conf;
+  private final ZKClientService zkClientService;
+
+  @Inject
+  public DefaultTransactionManagerProvider(Configuration conf, 
ZKClientService zkClientService) {
+this.conf = conf;
+this.zkClientService = zkClientService;
+  }
+
+  @Override
+  public TransactionManager get() {
+// Create a new injector every time since Guice services cannot be 
restarted TEPHRA-179
+Injector injector = Guice.createInjector(
--- End diff --

This is quite hacky in the way that usual provider doesn't create instance 
with a different injector. If all we need is a new instance, why not new it 
directly in here?


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #10: TEPHRA-179 Transaction service high avail...

2016-09-12 Thread chtyim 
Github user chtyim commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/10#discussion_r78406297
  
--- Diff: 
tephra-core/src/main/java/org/apache/tephra/distributed/TransactionService.java 
---
@@ -42,28 +45,64 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import javax.annotation.Nullable;
 
 /**
  *
  */
-public final class TransactionService extends InMemoryTransactionService {
+public class TransactionService extends AbstractService {
   private static final Logger LOG = 
LoggerFactory.getLogger(TransactionService.class);
   private LeaderElection leaderElection;
   private final ZKClient zkClient;
+  private final DiscoveryService discoveryService;
+  private final Provider txManagerProvider;
 
+  private final String serviceName;
+  private Cancellable cancelDiscovery;
+
+  // thrift server config
+  private final String address;
+  private final int port;
+  private final int threads;
+  private final int ioThreads;
+  private final int maxReadBufferBytes;
+
+  private TransactionManager txManager;
   private ThriftRPCServer server;
 
   @Inject
   public TransactionService(Configuration conf,
 ZKClient zkClient,
 DiscoveryService discoveryService,
 Provider 
txManagerProvider) {
-super(conf, discoveryService, txManagerProvider);
 this.zkClient = zkClient;
+this.discoveryService = discoveryService;
+this.txManagerProvider = txManagerProvider;
+
+serviceName = 
conf.get(TxConstants.Service.CFG_DATA_TX_DISCOVERY_SERVICE_NAME,
+   
TxConstants.Service.DEFAULT_DATA_TX_DISCOVERY_SERVICE_NAME);
+
+address = conf.get(TxConstants.Service.CFG_DATA_TX_BIND_ADDRESS, 
TxConstants.Service.DEFAULT_DATA_TX_BIND_ADDRESS);
+port = conf.getInt(TxConstants.Service.CFG_DATA_TX_BIND_PORT, 
TxConstants.Service.DEFAULT_DATA_TX_BIND_PORT);
+
+// Retrieve the number of threads for the service
+threads = conf.getInt(TxConstants.Service.CFG_DATA_TX_SERVER_THREADS,
+  
TxConstants.Service.DEFAULT_DATA_TX_SERVER_THREADS);
+ioThreads = 
conf.getInt(TxConstants.Service.CFG_DATA_TX_SERVER_IO_THREADS,
+
TxConstants.Service.DEFAULT_DATA_TX_SERVER_IO_THREADS);
+
+maxReadBufferBytes = 
conf.getInt(TxConstants.Service.CFG_DATA_TX_THRIFT_MAX_READ_BUFFER,
+ 
TxConstants.Service.DEFAULT_DATA_TX_THRIFT_MAX_READ_BUFFER);
+
+LOG.info("Configuring TransactionService" +
--- End diff --

Use the `{}` syntax instead of `+`.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #10: TEPHRA-179 Transaction service high avail...

2016-09-12 Thread chtyim 
Github user chtyim commented on a diff in the pull request:

https://github.com/apache/incubator-tephra/pull/10#discussion_r78405767
  
--- Diff: 
tephra-core/src/main/java/org/apache/tephra/distributed/TransactionService.java 
---
@@ -42,28 +45,64 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import javax.annotation.Nullable;
 
 /**
  *
  */
-public final class TransactionService extends InMemoryTransactionService {
+public class TransactionService extends AbstractService {
--- End diff --

Add a javadoc about this class to tell what does it do and when it should 
be used.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-tephra pull request #10: TEPHRA-179 Transaction service high avail...

2016-09-09 Thread poornachandra 
GitHub user poornachandra opened a pull request:

https://github.com/apache/incubator-tephra/pull/10

TEPHRA-179 Transaction service high availability changes

Restructuring the Transaction Service classes to allow for HA restart while 
binding Transaction Manager and other classes as singletons

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

$ git pull https://github.com/poornachandra/incubator-tephra 
feature/tx-service-ha

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

https://github.com/apache/incubator-tephra/pull/10.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 #10


commit 4b2bfe6a6733440fa73c7f5e00ff499662387911
Author: poorna 
Date:   2016-09-09T23:44:22Z

TEPHRA-179 Transaction service high availability changes

commit 7efff83009675a512f39cf8b0e94d1c6a1cde20a
Author: poorna 
Date:   2016-09-10T00:24:26Z

Add HA test




---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---