Repository navigation
feat: surface transaction abort reason on TxnConflictException #290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| # Multi-group, no-ACL cluster for the live transaction-abort-reason tests | ||
| # (io.dgraph.AbortReasonLiveTest). Two alpha groups (replicas=1) enable the | ||
| # predicate-move case; the single zero is restartable for the stale-startts case. | ||
| # | ||
| # Usage: | ||
| # make local-image # build dgraph/dgraph:local from this branch | ||
| # docker compose -f docker-compose.abort-reason.yml up -d | ||
| # TEST_GRPC_PORT=9180 \ | ||
| # TEST_ZERO_HTTP=localhost:6180 \ | ||
| # TEST_ZERO_RESTART_CMD="docker compose -f docker-compose.abort-reason.yml restart zero1" \ | ||
| # ./gradlew test --tests io.dgraph.AbortReasonLiveTest | ||
| # docker compose -f docker-compose.abort-reason.yml down | ||
| version: "3.5" | ||
| services: | ||
| zero1: | ||
| image: dgraph/dgraph:local | ||
| container_name: ar_zero1 | ||
| ports: | ||
| - 5180:5180 | ||
| - 6180:6180 | ||
| command: dgraph zero -o 100 --my=zero1:5180 --replicas=1 --logtostderr -v=2 --bindall | ||
|
|
||
| alpha1: | ||
| image: dgraph/dgraph:local | ||
| container_name: ar_alpha1 | ||
| ports: | ||
| - 8180:8180 | ||
| - 9180:9180 | ||
| command: | ||
| dgraph alpha -o 100 --my=alpha1:7180 --zero=zero1:5180 --logtostderr -v=2 --raft "idx=1; | ||
| group=1" --security "whitelist=0.0.0.0/0;" | ||
|
|
||
| alpha2: | ||
| image: dgraph/dgraph:local | ||
| container_name: ar_alpha2 | ||
| ports: | ||
| - 8182:8182 | ||
| - 9182:9182 | ||
| command: | ||
| dgraph alpha -o 102 --my=alpha2:7182 --zero=zero1:5180 --logtostderr -v=2 --raft "idx=2; | ||
| group=2" --security "whitelist=0.0.0.0/0;" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,277 @@ | ||
| /* | ||
| * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.dgraph; | ||
|
|
||
| import static org.testng.Assert.*; | ||
|
|
||
| import com.google.gson.Gson; | ||
| import com.google.protobuf.ByteString; | ||
| import io.dgraph.DgraphProto.Mutation; | ||
| import io.dgraph.DgraphProto.Operation; | ||
| import io.dgraph.DgraphProto.Response; | ||
| import io.dgraph.TxnConflictException.AbortReason; | ||
| import io.grpc.ManagedChannel; | ||
| import io.grpc.ManagedChannelBuilder; | ||
| import java.io.BufferedReader; | ||
| import java.io.InputStreamReader; | ||
| import java.net.HttpURLConnection; | ||
| import java.net.URL; | ||
| import java.util.Map; | ||
| import java.util.concurrent.TimeUnit; | ||
| import org.testng.SkipException; | ||
| import org.testng.annotations.AfterClass; | ||
| import org.testng.annotations.BeforeClass; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| /** | ||
| * Live cross-language end-to-end proof for the transaction-abort reason. Unlike {@link | ||
| * AbortReasonTest}, which feeds synthetic gRPC statuses into the parser, this test drives a real | ||
| * (locally patched) Dgraph server and asserts that each abort category propagates all the way to | ||
| * {@link TxnConflictException#getReason()}. This closes the loop the unit tests cannot — proving the | ||
| * server actually emits the categorized reason on the wire and the client parses it. | ||
| * | ||
| * <p>Configuration (system properties or environment variables): | ||
| * | ||
| * <ul> | ||
| * <li>{@code dgraph.test.host} / {@code TEST_HOSTNAME} — alpha host (default {@code localhost}) | ||
| * <li>{@code dgraph.test.port} / {@code TEST_GRPC_PORT} — alpha gRPC port (default {@code 9180}) | ||
| * <li>{@code dgraph.test.zeroHttp} / {@code TEST_ZERO_HTTP} — zero HTTP admin (e.g. {@code | ||
| * localhost:6180}); enables the predicate-move test (needs a multi-group cluster) | ||
| * <li>{@code dgraph.test.zeroRestartCmd} / {@code TEST_ZERO_RESTART_CMD} — shell command that | ||
| * restarts Zero; enables the stale-startts test | ||
| * </ul> | ||
| * | ||
| * Tests whose infrastructure is not configured are skipped (not failed), so the file is safe in the | ||
| * default run. Run explicitly with {@code --tests io.dgraph.AbortReasonLiveTest}. | ||
| */ | ||
| public class AbortReasonLiveTest { | ||
| private static final String HOST = conf("dgraph.test.host", "TEST_HOSTNAME", "localhost"); | ||
| private static final int PORT = | ||
| Integer.parseInt(conf("dgraph.test.port", "TEST_GRPC_PORT", "9180")); | ||
| private static final String ZERO_HTTP = conf("dgraph.test.zeroHttp", "TEST_ZERO_HTTP", null); | ||
| private static final String ZERO_RESTART_CMD = | ||
| conf("dgraph.test.zeroRestartCmd", "TEST_ZERO_RESTART_CMD", null); | ||
|
|
||
| private static ManagedChannel channel; | ||
| private static DgraphClient client; | ||
|
|
||
| private static String conf(String prop, String env, String dflt) { | ||
| String v = System.getProperty(prop); | ||
| if (v == null || v.isEmpty()) { | ||
| v = System.getenv(env); | ||
| } | ||
| return (v == null || v.isEmpty()) ? dflt : v; | ||
| } | ||
|
|
||
| @BeforeClass | ||
| public static void before() { | ||
| channel = ManagedChannelBuilder.forAddress(HOST, PORT).usePlaintext().build(); | ||
| client = new DgraphClient(DgraphGrpc.newStub(channel)); | ||
| client.alter(Operation.newBuilder().setDropAll(true).build()); | ||
| } | ||
|
Comment on lines
+69
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis
Authenticate or gate the default-port setup before With Do not add an unconditional 🤖 Prompt for AI Agents
|
||
|
|
||
| @AfterClass | ||
| public static void after() throws InterruptedException { | ||
| if (channel != null) { | ||
| channel.shutdown().awaitTermination(5, TimeUnit.SECONDS); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void liveConflictReportsConflictReason() { | ||
| // txn1 creates a node with a name. | ||
| Transaction txn1 = client.newTransaction(); | ||
| Mutation mu1 = | ||
| Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"Manish\"}")).build(); | ||
| Response assigned = txn1.mutate(mu1); | ||
| assertEquals(assigned.getUidsMap().size(), 1, "expected exactly one assigned uid"); | ||
| String uid = assigned.getUidsMap().values().iterator().next(); | ||
|
|
||
| // txn2 writes the same predicate on the same uid -> conflicts. | ||
| Transaction txn2 = client.newTransaction(); | ||
| Mutation mu2 = | ||
| Mutation.newBuilder() | ||
| .setSetJson(ByteString.copyFromUtf8("{\"uid\": \"" + uid + "\", \"name\": \"Manish\"}")) | ||
| .build(); | ||
| txn2.mutate(mu2); | ||
|
|
||
| // First commit wins; its commitTs is now greater than txn2's startTs. | ||
| txn1.commit(); | ||
|
|
||
| // Second commit must abort with the CONFLICT category, still retryable, with the | ||
| // full "conflict: ..." server message preserved. | ||
| try { | ||
| txn2.commit(); | ||
| fail("expected the conflicting second commit to throw TxnConflictException"); | ||
| } catch (TxnConflictException e) { | ||
| assertEquals( | ||
| e.getReason(), | ||
| AbortReason.CONFLICT, | ||
| "server-reported reason should parse to CONFLICT; full message: " + e.getMessage()); | ||
| assertTrue(e.isRetryable(), "conflict aborts are retryable"); | ||
| assertTrue( | ||
| e.getMessage().contains("conflict:"), | ||
| "full categorized server message should be preserved; got: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A transaction's start timestamp becomes "stale" when it falls below the oldest timestamp Zero | ||
| * can still validate against (its startTxnTs floor). Zero raises that floor on a leader change, | ||
| * and also when it trims its conflict map at a snapshot — the second is not a leader change at | ||
| * all, which is why the server message names both causes. | ||
| * | ||
| * <p>A leader change is simply the one that can be forced deterministically: open a transaction, | ||
| * then restart Zero (via the configured command). On restart Zero renews its lease and advances | ||
| * startTxnTs past every previously-leased start timestamp, so committing the now-old transaction | ||
| * aborts with STALE_STARTTS. | ||
| */ | ||
| @Test | ||
| public void liveStaleStartTsReportsStaleReason() throws Exception { | ||
| if (ZERO_RESTART_CMD == null) { | ||
| throw new SkipException( | ||
| "set dgraph.test.zeroRestartCmd / TEST_ZERO_RESTART_CMD to restart Zero"); | ||
| } | ||
|
|
||
| // Open a transaction so it gets a start ts that the restart will invalidate. | ||
| Transaction txn = client.newTransaction(); | ||
| txn.mutate( | ||
| Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"Manish\"}")).build()); | ||
|
|
||
| // Restart Zero; sleeps give the leader time to re-establish (lease renewal, hence the | ||
| // startTxnTs bump, runs on becoming leader). | ||
| runShell(ZERO_RESTART_CMD); | ||
| Thread.sleep(8000); | ||
|
|
||
| try { | ||
| txn.commit(); | ||
| fail("expected the stale commit to throw TxnConflictException"); | ||
| } catch (TxnConflictException e) { | ||
| assertEquals( | ||
| e.getReason(), | ||
| AbortReason.STALE_STARTTS, | ||
| "server-reported reason should parse to STALE_STARTTS; full message: " + e.getMessage()); | ||
| assertTrue( | ||
| e.getMessage().contains("stale-startts:"), | ||
| "full categorized server message should be preserved; got: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Moving a predicate's tablet to another group rejects commits that mutated it on the old group. | ||
| * We mutate "name" while its tablet is on the source group, move the tablet, then commit: the | ||
| * commit's predicate keys reference the old group, so Zero's checkPreds rejects it with the | ||
| * PREDICATE_MOVE category. | ||
| */ | ||
| @Test | ||
| public void livePredicateMoveReportsPredicateMoveReason() throws Exception { | ||
| if (ZERO_HTTP == null) { | ||
| throw new SkipException( | ||
| "set dgraph.test.zeroHttp / TEST_ZERO_HTTP and run a multi-group cluster"); | ||
| } | ||
|
|
||
| client.alter(Operation.newBuilder().setSchema("name: string @index(exact) .").build()); | ||
|
|
||
| // Seed so the "name" tablet exists and settles on some group. | ||
| Transaction seed = client.newTransaction(); | ||
| seed.mutate( | ||
| Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"seed\"}")).build()); | ||
| seed.commit(); | ||
| Thread.sleep(1000); | ||
|
|
||
| String src = groupOf("name"); | ||
| if (src == null || groupCount() < 2) { | ||
| throw new SkipException("need a multi-group cluster serving predicate 'name'"); | ||
| } | ||
| String dst = src.equals("1") ? "2" : "1"; | ||
|
|
||
| // Mutate "name" while it is on `src` (the txn's predicate keys reference `src`), don't commit. | ||
| Transaction txn = client.newTransaction(); | ||
| txn.mutate( | ||
| Mutation.newBuilder().setSetJson(ByteString.copyFromUtf8("{\"name\": \"Manish\"}")).build()); | ||
|
|
||
| // Move the tablet and wait for the move to complete. | ||
| httpGet("http://" + ZERO_HTTP + "/moveTablet?tablet=name&group=" + dst); | ||
| long deadline = System.currentTimeMillis() + 60_000; | ||
| while (System.currentTimeMillis() < deadline && !dst.equals(groupOf("name"))) { | ||
| Thread.sleep(1000); | ||
| } | ||
| assertEquals(groupOf("name"), dst, "tablet move did not complete"); | ||
|
|
||
| try { | ||
| txn.commit(); | ||
| fail("expected the post-move commit to throw TxnConflictException"); | ||
| } catch (TxnConflictException e) { | ||
| assertEquals( | ||
| e.getReason(), | ||
| AbortReason.PREDICATE_MOVE, | ||
| "server-reported reason should parse to PREDICATE_MOVE; full message: " + e.getMessage()); | ||
| assertTrue( | ||
| e.getMessage().contains("predicate-move:"), | ||
| "full categorized server message should be preserved; got: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| // --- helpers --- | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static Map<String, Object> zeroState() throws Exception { | ||
| String body = httpGet("http://" + ZERO_HTTP + "/state"); | ||
| return new Gson().fromJson(body, Map.class); | ||
| } | ||
|
|
||
| /** Returns the group id serving the given predicate (matching the namespaced tablet key). */ | ||
| @SuppressWarnings("unchecked") | ||
| private static String groupOf(String pred) throws Exception { | ||
| Map<String, Object> groups = (Map<String, Object>) zeroState().get("groups"); | ||
| if (groups == null) { | ||
| return null; | ||
| } | ||
| for (Map.Entry<String, Object> e : groups.entrySet()) { | ||
| Object tabletsObj = ((Map<String, Object>) e.getValue()).get("tablets"); | ||
| if (tabletsObj instanceof Map) { | ||
| for (String tablet : ((Map<String, Object>) tabletsObj).keySet()) { | ||
| if (tablet.equals(pred) || tablet.endsWith("-" + pred)) { | ||
| return e.getKey(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| private static int groupCount() throws Exception { | ||
| Map<String, Object> groups = (Map<String, Object>) zeroState().get("groups"); | ||
| return groups == null ? 0 : groups.size(); | ||
| } | ||
|
|
||
| private static String httpGet(String urlStr) throws Exception { | ||
| HttpURLConnection conn = (HttpURLConnection) new URL(urlStr).openConnection(); | ||
| conn.setRequestMethod("GET"); | ||
| try (BufferedReader rd = new BufferedReader(new InputStreamReader(conn.getInputStream()))) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| String line; | ||
| while ((line = rd.readLine()) != null) { | ||
| sb.append(line); | ||
| } | ||
| return sb.toString(); | ||
| } finally { | ||
| conn.disconnect(); | ||
| } | ||
| } | ||
|
|
||
| private static void runShell(String cmd) throws Exception { | ||
| Process p = new ProcessBuilder("bash", "-c", cmd).inheritIO().start(); | ||
| if (!p.waitFor(60, TimeUnit.SECONDS)) { | ||
| p.destroyForcibly(); | ||
| throw new RuntimeException("zero restart command timed out: " + cmd); | ||
| } | ||
| if (p.exitValue() != 0) { | ||
| throw new RuntimeException("zero restart command failed (" + p.exitValue() + "): " + cmd); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: dgraph-io/dgraph4j
Length of output: 9613
🏁 Script executed:
Repository: dgraph-io/dgraph4j
Length of output: 8335
Skip only when the cluster is unreachable.
before()callsclient.alter(...setDropAll(true)...)without a guard. When no cluster is reachable, the gRPC call fails withUNAVAILABLE.Exceptions.translatemaps that status toConnectionException, so the failure escapesbefore()as a class configuration failure. The class-level Javadoc says tests are skipped when infrastructure is missing, butbefore()does not follow that rule.Catching
RuntimeExceptionwould skip every failure fromalter.DgraphExceptionextendsStatusRuntimeException, so authentication, permission, and server-side DropAll errors from a reachable cluster would also become skips. The integration cluster runs with ACL enabled, and the shared fixture logs in asgrootbefore DropAll. A reachable cluster that rejects an unauthenticated DropAll would therefore be reported as skipped. Catch only the unreachable-cluster type.🐛 Suggested fix
@BeforeClass public static void before() { channel = ManagedChannelBuilder.forAddress(HOST, PORT).usePlaintext().build(); client = new DgraphClient(DgraphGrpc.newStub(channel)); - client.alter(Operation.newBuilder().setDropAll(true).build()); + try { + client.alter(Operation.newBuilder().setDropAll(true).build()); + } catch (ConnectionException e) { + channel.shutdownNow(); + throw new SkipException("no Dgraph cluster reachable at " + HOST + ":" + PORT, e); + } }🤖 Prompt for AI Agents