Uploaded image for project: 'Tephra'
  1. Tephra
  2. TEPHRA-228

Introduce client ID that can be used to track requests per client

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.12.0-incubating
    • Component/s: api, core
    • Labels:
      None

      Description

      Today transaction manager does not have details of what transaction belongs to which client. For instance when a there are a lot of invalid transactions generated by a client, it is not easy to say which client generated the invalid transactions. Transaction manager just logs a message saying the transaction ID was invalidated. This makes debugging very difficult since there is no easy way to map the transaction ID to a program.

      Transaction APIs should allow clients to pass in client IDs for every start transaction call. Transaction manager can store this client ID as part of the transaction metadata. And when transaction manager logs messages with transaction ID, it can include the client ID in the message.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/incubator-tephra/pull/42
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117851160

          — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java —
          @@ -125,15 +130,20 @@ public static void doMain(boolean verbose, Configuration conf) throws Exception
          }
          }

          + public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider) {
          — End diff –

          Please add javadocs for the public constructor.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117851160 — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java — @@ -125,15 +130,20 @@ public static void doMain(boolean verbose, Configuration conf) throws Exception } } + public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider) { — End diff – Please add javadocs for the public constructor.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117847180

          — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java —
          @@ -130,10 +134,11 @@ public static void doMain(boolean verbose, Configuration conf) throws Exception

          • for service discovery. Otherwise it will look for the port in the
          • config and use localhost.
          • @param config a configuration containing the zookeeper properties
            + * @param clientId id of the client that identifies it when it starts a transaction
            */
            @Inject
          • public TransactionServiceClient(Configuration config,
          • ThriftClientProvider clientProvider) {
            + public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider,
              • End diff –

          @poornachandra Addressed comment in the commit https://github.com/apache/incubator-tephra/pull/42/commits/0279658b3b0718298ccfe363343bce182e4ba655. Please take a look again.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117847180 — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java — @@ -130,10 +134,11 @@ public static void doMain(boolean verbose, Configuration conf) throws Exception for service discovery. Otherwise it will look for the port in the config and use localhost. @param config a configuration containing the zookeeper properties + * @param clientId id of the client that identifies it when it starts a transaction */ @Inject public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider) { + public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider, End diff – @poornachandra Addressed comment in the commit https://github.com/apache/incubator-tephra/pull/42/commits/0279658b3b0718298ccfe363343bce182e4ba655 . Please take a look again.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117843422

          — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java —
          @@ -130,10 +134,11 @@ public static void doMain(boolean verbose, Configuration conf) throws Exception

          • for service discovery. Otherwise it will look for the port in the
          • config and use localhost.
          • @param config a configuration containing the zookeeper properties
            + * @param clientId id of the client that identifies it when it starts a transaction
            */
            @Inject
          • public TransactionServiceClient(Configuration config,
          • ThriftClientProvider clientProvider) {
            + public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider,
              • End diff –

          Some clients like Apache Phoenix do not use the guice modules. They instantiate `TransactionServiceClient` directly. It will be good to have the existing constructor too and assign the default value for the clientId in it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117843422 — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java — @@ -130,10 +134,11 @@ public static void doMain(boolean verbose, Configuration conf) throws Exception for service discovery. Otherwise it will look for the port in the config and use localhost. @param config a configuration containing the zookeeper properties + * @param clientId id of the client that identifies it when it starts a transaction */ @Inject public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider) { + public TransactionServiceClient(Configuration config, ThriftClientProvider clientProvider, End diff – Some clients like Apache Phoenix do not use the guice modules. They instantiate `TransactionServiceClient` directly. It will be good to have the existing constructor too and assign the default value for the clientId in it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

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

          @poornachandra Thanks for the comments. Addressed them.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/incubator-tephra/pull/42 @poornachandra Thanks for the comments. Addressed them.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117841761

          — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/TransactionDistributedModule.java —
          @@ -35,13 +34,24 @@
          import org.apache.tephra.persist.TransactionStateStorage;
          import org.apache.tephra.snapshot.SnapshotCodecProvider;

          +import java.lang.management.ManagementFactory;
          +
          /**

          • Guice bindings for running in distributed mode on a cluster.
            */
            -final class TransactionDistributedModule extends AbstractModule {
            +final class TransactionDistributedModule extends ClientIdAwareTransactionModule {
            +
            + public TransactionDistributedModule() {
            + super(ManagementFactory.getRuntimeMXBean().getName());
              • End diff –

          In that case they can provide the client id since the default constructor won't be present, right?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117841761 — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/TransactionDistributedModule.java — @@ -35,13 +34,24 @@ import org.apache.tephra.persist.TransactionStateStorage; import org.apache.tephra.snapshot.SnapshotCodecProvider; +import java.lang.management.ManagementFactory; + /** Guice bindings for running in distributed mode on a cluster. */ -final class TransactionDistributedModule extends AbstractModule { +final class TransactionDistributedModule extends ClientIdAwareTransactionModule { + + public TransactionDistributedModule() { + super(ManagementFactory.getRuntimeMXBean().getName()); End diff – In that case they can provide the client id since the default constructor won't be present, right?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117838945

          — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/ClientIdAwareTransactionModule.java —
          @@ -0,0 +1,41 @@
          +/*
          + * 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.inject.AbstractModule;
          +import com.google.inject.name.Names;
          +import org.apache.tephra.TransactionSystemClient;
          +import org.apache.tephra.distributed.TransactionServiceClient;
          +
          +/**
          + * Guice module that binds the clientId to be used in

          {@link TransactionSystemClient}

          .
          + */
          +public abstract class ClientIdAwareTransactionModule extends AbstractModule {
          — End diff –

          Sure ok.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117838945 — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/ClientIdAwareTransactionModule.java — @@ -0,0 +1,41 @@ +/* + * 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.inject.AbstractModule; +import com.google.inject.name.Names; +import org.apache.tephra.TransactionSystemClient; +import org.apache.tephra.distributed.TransactionServiceClient; + +/** + * Guice module that binds the clientId to be used in {@link TransactionSystemClient} . + */ +public abstract class ClientIdAwareTransactionModule extends AbstractModule { — End diff – Sure ok.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117838903

          — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/TransactionDistributedModule.java —
          @@ -35,13 +34,24 @@
          import org.apache.tephra.persist.TransactionStateStorage;
          import org.apache.tephra.snapshot.SnapshotCodecProvider;

          +import java.lang.management.ManagementFactory;
          +
          /**

          • Guice bindings for running in distributed mode on a cluster.
            */
            -final class TransactionDistributedModule extends AbstractModule {
            +final class TransactionDistributedModule extends ClientIdAwareTransactionModule {
            +
            + public TransactionDistributedModule() {
            + super(ManagementFactory.getRuntimeMXBean().getName());
              • End diff –

          @poornachandra There is no guarantee that TransactionModules().getDistributedModule() is used to get an instance of TransactionDistributedModule. One can just create an instance of this class.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117838903 — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/TransactionDistributedModule.java — @@ -35,13 +34,24 @@ import org.apache.tephra.persist.TransactionStateStorage; import org.apache.tephra.snapshot.SnapshotCodecProvider; +import java.lang.management.ManagementFactory; + /** Guice bindings for running in distributed mode on a cluster. */ -final class TransactionDistributedModule extends AbstractModule { +final class TransactionDistributedModule extends ClientIdAwareTransactionModule { + + public TransactionDistributedModule() { + super(ManagementFactory.getRuntimeMXBean().getName()); End diff – @poornachandra There is no guarantee that TransactionModules().getDistributedModule() is used to get an instance of TransactionDistributedModule. One can just create an instance of this class.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117826105

          — Diff: tephra-core/src/main/java/org/apache/tephra/TxConstants.java —
          @@ -120,6 +120,11 @@
          public static final boolean DEFAULT_READ_NON_TX_DATA = false;

          /**
          + * Client id that is used if a client doesn't provide one while starting a transaction.
          + */
          + public static final String DEFAULT_CLIENTID = "unknown";
          — End diff –

          This constant can be defined in `TransactionManager` class now, right?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117826105 — Diff: tephra-core/src/main/java/org/apache/tephra/TxConstants.java — @@ -120,6 +120,11 @@ public static final boolean DEFAULT_READ_NON_TX_DATA = false; /** + * Client id that is used if a client doesn't provide one while starting a transaction. + */ + public static final String DEFAULT_CLIENTID = "unknown"; — End diff – This constant can be defined in `TransactionManager` class now, right?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117834468

          — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/TransactionDistributedModule.java —
          @@ -35,13 +34,24 @@
          import org.apache.tephra.persist.TransactionStateStorage;
          import org.apache.tephra.snapshot.SnapshotCodecProvider;

          +import java.lang.management.ManagementFactory;
          +
          /**

          • Guice bindings for running in distributed mode on a cluster.
            */
            -final class TransactionDistributedModule extends AbstractModule {
            +final class TransactionDistributedModule extends ClientIdAwareTransactionModule {
            +
            + public TransactionDistributedModule() {
            + super(ManagementFactory.getRuntimeMXBean().getName());
              • End diff –

          This default is already defined in `TransactionModules`. Do we still need it here?
          Same for `TransactionInMemoryModule` and `TransactionLocalModule`.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117834468 — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/TransactionDistributedModule.java — @@ -35,13 +34,24 @@ import org.apache.tephra.persist.TransactionStateStorage; import org.apache.tephra.snapshot.SnapshotCodecProvider; +import java.lang.management.ManagementFactory; + /** Guice bindings for running in distributed mode on a cluster. */ -final class TransactionDistributedModule extends AbstractModule { +final class TransactionDistributedModule extends ClientIdAwareTransactionModule { + + public TransactionDistributedModule() { + super(ManagementFactory.getRuntimeMXBean().getName()); End diff – This default is already defined in `TransactionModules`. Do we still need it here? Same for `TransactionInMemoryModule` and `TransactionLocalModule`.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117834066

          — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/ClientIdAwareTransactionModule.java —
          @@ -0,0 +1,41 @@
          +/*
          + * 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.inject.AbstractModule;
          +import com.google.inject.name.Names;
          +import org.apache.tephra.TransactionSystemClient;
          +import org.apache.tephra.distributed.TransactionServiceClient;
          +
          +/**
          + * Guice module that binds the clientId to be used in

          {@link TransactionSystemClient}

          .
          + */
          +public abstract class ClientIdAwareTransactionModule extends AbstractModule {
          — End diff –

          Having `ClientIdAwareTransactionModule` as the parent class of Transaction modules does not add much value. Let's move the binding to the child modules.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117834066 — Diff: tephra-core/src/main/java/org/apache/tephra/runtime/ClientIdAwareTransactionModule.java — @@ -0,0 +1,41 @@ +/* + * 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.inject.AbstractModule; +import com.google.inject.name.Names; +import org.apache.tephra.TransactionSystemClient; +import org.apache.tephra.distributed.TransactionServiceClient; + +/** + * Guice module that binds the clientId to be used in {@link TransactionSystemClient} . + */ +public abstract class ClientIdAwareTransactionModule extends AbstractModule { — End diff – Having `ClientIdAwareTransactionModule` as the parent class of Transaction modules does not add much value. Let's move the binding to the child modules.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117827109

          — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java —
          @@ -50,6 +51,7 @@

          • A tx service client
            */
            public class TransactionServiceClient implements TransactionSystemClient {
            + public static final String CLIENT_ID = "clientId";
              • End diff –

          It would be better to move this definition to `TxConstants` since this is to be used by external programs. Also call it as `tephra.client.id` to disambiguate.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117827109 — Diff: tephra-core/src/main/java/org/apache/tephra/distributed/TransactionServiceClient.java — @@ -50,6 +51,7 @@ A tx service client */ public class TransactionServiceClient implements TransactionSystemClient { + public static final String CLIENT_ID = "clientId"; End diff – It would be better to move this definition to `TxConstants` since this is to be used by external programs. Also call it as `tephra.client.id` to disambiguate.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

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

          @poornachandra Please review the last commit. I have addressed your comment about including hostname-pid as the default client id.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/incubator-tephra/pull/42 @poornachandra Please review the last commit. I have addressed your comment about including hostname-pid as the default client id.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

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

          @poornachandra Injecting ClientId in TransactionServiceClient now instead of changing the API of TransactionSystemClient. Please review when you get a chance.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/incubator-tephra/pull/42 @poornachandra Injecting ClientId in TransactionServiceClient now instead of changing the API of TransactionSystemClient. Please review when you get a chance.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

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

          @poornachandra Can you please review the last commit. I have addressed your comment about TransactionContext change (deprecated old constructors and introduced a new one with the clientId)

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/incubator-tephra/pull/42 @poornachandra Can you please review the last commit. I have addressed your comment about TransactionContext change (deprecated old constructors and introduced a new one with the clientId)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117557607

          — Diff: tephra-hbase-compat-0.98/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java —
          @@ -127,7 +126,7 @@ public TransactionProcessor() {
          public void start(CoprocessorEnvironment e) throws IOException {
          if (e instanceof RegionCoprocessorEnvironment) {
          RegionCoprocessorEnvironment env = (RegionCoprocessorEnvironment) e;

          • Supplier<TransactionStateCache> cacheSupplier = getTransactionStateCacheSupplier(env);
            + this.cacheSupplier = getTransactionStateCacheSupplier(env);
              • End diff –

          Missed this change in https://github.com/apache/incubator-tephra/pull/41 for this compat-module. Hence fixing it here.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117557607 — Diff: tephra-hbase-compat-0.98/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java — @@ -127,7 +126,7 @@ public TransactionProcessor() { public void start(CoprocessorEnvironment e) throws IOException { if (e instanceof RegionCoprocessorEnvironment) { RegionCoprocessorEnvironment env = (RegionCoprocessorEnvironment) e; Supplier<TransactionStateCache> cacheSupplier = getTransactionStateCacheSupplier(env); + this.cacheSupplier = getTransactionStateCacheSupplier(env); End diff – Missed this change in https://github.com/apache/incubator-tephra/pull/41 for this compat-module. Hence fixing it here.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117552592

          — Diff: tephra-hbase-compat-0.98/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java —
          @@ -127,7 +126,7 @@ public TransactionProcessor() {
          public void start(CoprocessorEnvironment e) throws IOException {
          if (e instanceof RegionCoprocessorEnvironment) {
          RegionCoprocessorEnvironment env = (RegionCoprocessorEnvironment) e;

          • Supplier<TransactionStateCache> cacheSupplier = getTransactionStateCacheSupplier(env);
            + this.cacheSupplier = getTransactionStateCacheSupplier(env);
              • End diff –

          Unrelated change?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117552592 — Diff: tephra-hbase-compat-0.98/src/main/java/org/apache/tephra/hbase/coprocessor/TransactionProcessor.java — @@ -127,7 +126,7 @@ public TransactionProcessor() { public void start(CoprocessorEnvironment e) throws IOException { if (e instanceof RegionCoprocessorEnvironment) { RegionCoprocessorEnvironment env = (RegionCoprocessorEnvironment) e; Supplier<TransactionStateCache> cacheSupplier = getTransactionStateCacheSupplier(env); + this.cacheSupplier = getTransactionStateCacheSupplier(env); End diff – Unrelated change?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117552141

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -1378,17 +1403,32 @@ public TransactionType getTransactionType() {
          private final long expiration;
          private final InProgressType type;
          private final LongArrayList checkpointWritePointers;
          + private final String clientId;
          — End diff –

          It is okay not to add it now since we are not persisting it. Add a comment in the code.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117552141 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -1378,17 +1403,32 @@ public TransactionType getTransactionType() { private final long expiration; private final InProgressType type; private final LongArrayList checkpointWritePointers; + private final String clientId; — End diff – It is okay not to add it now since we are not persisting it. Add a comment in the code.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117367563

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -763,15 +781,22 @@ private long getNextWritePointer() {

          • transaction moves it to the invalid list because we assume that its writes cannot be rolled back.
            */
            public Transaction startLong() { + return startLong(null); + }

            +
            + /**
            + * Starts a long transaction with a client id.
            + */
            + public Transaction startLong(String clientId) {

              • End diff –

          Added Preconditions null check for clientId.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117367563 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -763,15 +781,22 @@ private long getNextWritePointer() { transaction moves it to the invalid list because we assume that its writes cannot be rolled back. */ public Transaction startLong() { + return startLong(null); + } + + /** + * Starts a long transaction with a client id. + */ + public Transaction startLong(String clientId) { End diff – Added Preconditions null check for clientId.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117367472

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java —
          @@ -44,11 +44,33 @@
          Transaction startShort(int timeout);
          — End diff –

          Done.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117367472 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java — @@ -44,11 +44,33 @@ Transaction startShort(int timeout); — End diff – Done.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on the issue:

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

          @poornachandra Addressed comments. Please take a look.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on the issue: https://github.com/apache/incubator-tephra/pull/42 @poornachandra Addressed comments. Please take a look.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117363141

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -763,15 +781,22 @@ private long getNextWritePointer() {

          • transaction moves it to the invalid list because we assume that its writes cannot be rolled back.
            */
            public Transaction startLong() { + return startLong(null); + }

            +
            + /**
            + * Starts a long transaction with a client id.
            + */
            + public Transaction startLong(String clientId) {

              • End diff –

          @poornachandra Sure sounds good. I will add a Precondition check for String not being null.

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117363141 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -763,15 +781,22 @@ private long getNextWritePointer() { transaction moves it to the invalid list because we assume that its writes cannot be rolled back. */ public Transaction startLong() { + return startLong(null); + } + + /** + * Starts a long transaction with a client id. + */ + public Transaction startLong(String clientId) { End diff – @poornachandra Sure sounds good. I will add a Precondition check for String not being null.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117338264

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -763,15 +781,22 @@ private long getNextWritePointer() {

          • transaction moves it to the invalid list because we assume that its writes cannot be rolled back.
            */
            public Transaction startLong() { + return startLong(null); + }

            +
            + /**
            + * Starts a long transaction with a client id.
            + */
            + public Transaction startLong(String clientId) {

              • End diff –

          Eventually we want all client's to pass in a client i, hence the deprecation. If we allow nulls then that defeats the purpose, no?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117338264 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -763,15 +781,22 @@ private long getNextWritePointer() { transaction moves it to the invalid list because we assume that its writes cannot be rolled back. */ public Transaction startLong() { + return startLong(null); + } + + /** + * Starts a long transaction with a client id. + */ + public Transaction startLong(String clientId) { End diff – Eventually we want all client's to pass in a client i, hence the deprecation. If we allow nulls then that defeats the purpose, no?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117324833

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -763,15 +781,22 @@ private long getNextWritePointer() {

          • transaction moves it to the invalid list because we assume that its writes cannot be rolled back.
            */
            public Transaction startLong() { + return startLong(null); + }

            +
            + /**
            + * Starts a long transaction with a client id.
            + */
            + public Transaction startLong(String clientId) {

              • End diff –

          Should I add Nullable annotation if we decide to allow null for clientId?

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117324833 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -763,15 +781,22 @@ private long getNextWritePointer() { transaction moves it to the invalid list because we assume that its writes cannot be rolled back. */ public Transaction startLong() { + return startLong(null); + } + + /** + * Starts a long transaction with a client id. + */ + public Transaction startLong(String clientId) { End diff – Should I add Nullable annotation if we decide to allow null for clientId?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117322759

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -1378,17 +1403,32 @@ public TransactionType getTransactionType() {
          private final long expiration;
          private final InProgressType type;
          private final LongArrayList checkpointWritePointers;
          + private final String clientId;
          — End diff –

          Not sure if we can add it to equals/hashCode. The unit tests fail when trying to compare the state before persistence and after recovery and won't be the same if we include clientId in comparison. Any reason why we should definitely include it? Or should we include it and adjust the test that does the comparison?

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117322759 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -1378,17 +1403,32 @@ public TransactionType getTransactionType() { private final long expiration; private final InProgressType type; private final LongArrayList checkpointWritePointers; + private final String clientId; — End diff – Not sure if we can add it to equals/hashCode. The unit tests fail when trying to compare the state before persistence and after recovery and won't be the same if we include clientId in comparison. Any reason why we should definitely include it? Or should we include it and adjust the test that does the comparison?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gokulavasan commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117316435

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -763,15 +781,22 @@ private long getNextWritePointer() {

          • transaction moves it to the invalid list because we assume that its writes cannot be rolled back.
            */
            public Transaction startLong() { + return startLong(null); + }

            +
            + /**
            + * Starts a long transaction with a client id.
            + */
            + public Transaction startLong(String clientId) {

              • End diff –

          If we are going to deprecate the clientId-less methods, should we allow null here?

          Show
          githubbot ASF GitHub Bot added a comment - Github user gokulavasan commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117316435 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -763,15 +781,22 @@ private long getNextWritePointer() { transaction moves it to the invalid list because we assume that its writes cannot be rolled back. */ public Transaction startLong() { + return startLong(null); + } + + /** + * Starts a long transaction with a client id. + */ + public Transaction startLong(String clientId) { End diff – If we are going to deprecate the clientId-less methods, should we allow null here?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117117388

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java —
          @@ -44,11 +44,33 @@
          Transaction startShort(int timeout);
          — End diff –

          It would be good to deprecate all the start methods that do not take in a client id.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117117388 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java — @@ -44,11 +44,33 @@ Transaction startShort(int timeout); — End diff – It would be good to deprecate all the start methods that do not take in a client id.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117127048

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -347,15 +346,17 @@ private void cleanupTimedOutTransactions() {
          long currentTime = System.currentTimeMillis();
          Map<Long, InProgressType> timedOut = Maps.newHashMap();
          for (Map.Entry<Long, InProgressTx> tx : inProgress.entrySet()) {

          • long expiration = tx.getValue().getExpiration();
            + InProgressTx inProgressTx = tx.getValue();
            + long expiration = inProgressTx.getExpiration();
            if (expiration >= 0L && currentTime > expiration) {
            // timed out, remember tx id (can't remove while iterating over entries)
          • timedOut.put(tx.getKey(), tx.getValue().getType());
          • LOG.info("Tx invalid list: added tx {} because of timeout", tx.getKey());
            + timedOut.put(tx.getKey(), inProgressTx.getType());
            + String clientId = inProgressTx.getClientId() != null ? inProgressTx.getClientId() : "unknown";
            + LOG.info("Tx invalid list: added tx {} belonging to client '{}' because of timeout.",
              • End diff –

          Let's add the client id to the log message in `doInvalidate` too.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117127048 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -347,15 +346,17 @@ private void cleanupTimedOutTransactions() { long currentTime = System.currentTimeMillis(); Map<Long, InProgressType> timedOut = Maps.newHashMap(); for (Map.Entry<Long, InProgressTx> tx : inProgress.entrySet()) { long expiration = tx.getValue().getExpiration(); + InProgressTx inProgressTx = tx.getValue(); + long expiration = inProgressTx.getExpiration(); if (expiration >= 0L && currentTime > expiration) { // timed out, remember tx id (can't remove while iterating over entries) timedOut.put(tx.getKey(), tx.getValue().getType()); LOG.info("Tx invalid list: added tx {} because of timeout", tx.getKey()); + timedOut.put(tx.getKey(), inProgressTx.getType()); + String clientId = inProgressTx.getClientId() != null ? inProgressTx.getClientId() : "unknown"; + LOG.info("Tx invalid list: added tx {} belonging to client '{}' because of timeout.", End diff – Let's add the client id to the log message in `doInvalidate` too.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117121506

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java —
          @@ -44,11 +44,33 @@
          Transaction startShort(int timeout);

          /**
          + * Starts a new short transaction.
          + * @param clientId id of the client
          — End diff –

          Let's be a little more descriptive, something like - `An id that can be used to identify the client starting the transaction`

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117121506 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionSystemClient.java — @@ -44,11 +44,33 @@ Transaction startShort(int timeout); /** + * Starts a new short transaction. + * @param clientId id of the client — End diff – Let's be a little more descriptive, something like - `An id that can be used to identify the client starting the transaction`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117129143

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -763,15 +781,22 @@ private long getNextWritePointer() {

          • transaction moves it to the invalid list because we assume that its writes cannot be rolled back.
            */
            public Transaction startLong() { + return startLong(null); + }

            +
            + /**
            + * Starts a long transaction with a client id.
            + */
            + public Transaction startLong(String clientId) {

              • End diff –

          Should we throw an exception if `clientId` is null?

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117129143 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -763,15 +781,22 @@ private long getNextWritePointer() { transaction moves it to the invalid list because we assume that its writes cannot be rolled back. */ public Transaction startLong() { + return startLong(null); + } + + /** + * Starts a long transaction with a client id. + */ + public Transaction startLong(String clientId) { End diff – Should we throw an exception if `clientId` is null?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user poornachandra commented on a diff in the pull request:

          https://github.com/apache/incubator-tephra/pull/42#discussion_r117127771

          — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java —
          @@ -1378,17 +1403,32 @@ public TransactionType getTransactionType() {
          private final long expiration;
          private final InProgressType type;
          private final LongArrayList checkpointWritePointers;
          + private final String clientId;
          — End diff –

          Let's add the `clientId` to `toString()`, `equals()` and `hashCode()` methods.

          Show
          githubbot ASF GitHub Bot added a comment - Github user poornachandra commented on a diff in the pull request: https://github.com/apache/incubator-tephra/pull/42#discussion_r117127771 — Diff: tephra-core/src/main/java/org/apache/tephra/TransactionManager.java — @@ -1378,17 +1403,32 @@ public TransactionType getTransactionType() { private final long expiration; private final InProgressType type; private final LongArrayList checkpointWritePointers; + private final String clientId; — End diff – Let's add the `clientId` to `toString()`, `equals()` and `hashCode()` methods.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user gokulavasan opened a pull request:

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

          TEPHRA-228 Adding the ability to pass-in a clientId during the start …

          …of a transaction which is logged when the transaction gets invalidated during time out.

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

          $ git pull https://github.com/gokulavasan/incubator-tephra feature/tephra-228

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

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


          commit 46760aa42b790d2f030f588cce0648b2dd94369a
          Author: Gokul Gunasekaran <gokul@cask.co>
          Date: 2017-05-16T01:06:10Z

          TEPHRA-228 Adding the ability to pass-in a clientId during the start of a transaction which is logged when the transaction gets invalidated during time out.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user gokulavasan opened a pull request: https://github.com/apache/incubator-tephra/pull/42 TEPHRA-228 Adding the ability to pass-in a clientId during the start … …of a transaction which is logged when the transaction gets invalidated during time out. You can merge this pull request into a Git repository by running: $ git pull https://github.com/gokulavasan/incubator-tephra feature/tephra-228 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-tephra/pull/42.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 #42 commit 46760aa42b790d2f030f588cce0648b2dd94369a Author: Gokul Gunasekaran <gokul@cask.co> Date: 2017-05-16T01:06:10Z TEPHRA-228 Adding the ability to pass-in a clientId during the start of a transaction which is logged when the transaction gets invalidated during time out.

            People

            • Assignee:
              gokulavasan Gokul Gunasekaran
              Reporter:
              poornachandra Poorna Chandra
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development