opentracing-contrib / java-jdbc

OpenTracing Instrumentation for JDBC
Apache License 2.0
82 stars 56 forks source link

TracerPreparedStatement.executeBatch() is failing. #37

Closed sudiptasish closed 5 years ago

sudiptasish commented 5 years ago

First, let’s revisit the interface/class hierarchy:

  1. java.sql.PreparedStatement extends java.sql.Statement
  2. io.opentracing.contrib.jdbc.TracingStatement implements java.sql.Statement
  3. io.opentracing.contrib.jdbc.TracingPreparedStatement implements java.sql.PreparedStatement
  4. io.opentracing.contrib.jdbc.TracingPreparedStatement extends io.opentracing.contrib.jdbc.TracingStatement

Some fact:

  1. executeBatch() method is defined in java.sql.Statement interface. And io.opentracing.contrib.jdbc.TracingStatement class has implemented this method.
  2. Both TracingPreparedStatement and TracingStatement class have their independent Tracer variable (instance level).

Now, let’s look at the below piece of code. It adds 10 records to a batch and send all 10 records to the database at one go. (Read the inline comments for better understanding) :

// Get the Connection object, which already has the Tracer object in it. // getConnection() object is an utility method to return a TracingConnection with a valid Tracer object in it. (I have not given the implementation here). // This connection (conn) is of type io.opentracing.contrib.jdbc.TracingConnection. Connection conn = getConnection();

// The same Tracer object will be passed internally to the newly created PreparedStatement (pstmt) object. // This PreparedStatement is nothing but of type io.opentracing.contrib.jdbc.TracingPreparedStatement. PreparedStatement pstmt = conn.prepareStatement(“”);

// Start the batch operation. for (int i = 0; i < 10; i ++) { pstmt.setXXX(…); pstmt.setXXX(…); pstmt.setXXX(…);

pstmt.addBatch();

} // It calls io.opentracing.contrib.jdbc.TracingStatement.executeBatch() [note fact #1]. // Because TracingStatement has its own copy of Tracer object [note fact #2], which is // still in uninitialized state, the call fails. int[] count = pstmt.executeBatch();

The last line pstmt.executeBatch() is failing, because it's Tracer object is null.


Because TracingPreparedStatement is extending TracingStatement, I feel they should share the same Tracer object defined in the TracingStatement. Having a separate Tracer object in the child class (TracingPreparedStatement) is causing the issue.

malafeev commented 5 years ago

TracingConnection passes owned instance of tracer to TracingPreparedStatement and TracingStatement. So they all have the same tracer object.

malafeev commented 5 years ago

Could you provide JUnit test for your case (like in https://github.com/opentracing-contrib/java-jdbc/blob/master/src/test/java/io/opentracing/contrib/jdbc/JdbcTest.java) or sample application?

malafeev commented 5 years ago

I created PR #38 based on what you described, actually you are right, fields of TracingStatement should be reused. Although I still don't understand why executeBatch() is failing for you.

sudiptasish commented 5 years ago

Note that, we are not using GlobalTracer to store our platform Tracer instance.

Before we go into the test case, let me try to explain it further. Consider the below 3 classes:

1.

public class Base {
    private Tracer tracer;

    public Base(Tracer tracer) {
        this.tracer = tracer;
    }

    public void executeBatch() {
        System.out.println("Tracer Object: " + tracer);
    }
}

2.

public class Child extends Base {
    private Tracer tracer;

    public Child(Tracer tracer) {
        this.tracer = tracer;
    }
}
  1. public class Moderator {
    private Tracer tracer;
    
    public void setTracer(Tracer tracer) {
        this.tracer = tracer;
    }
    
    public Child prepareChild() {
        return new Child(tracer);
    }
    }
  2. Here is the Main class:

    public class Main {
    public static void main(String[] args) {
        Tracer tracer = new Tracer();
        Moderator m = new Moderator();
        m.setTracer(tracer);
    
        // The below call will pass the Tracer object to the Child object.
        // So the "child" now has valid non-null tracer object.
        Child child = m.prepareChild();    
    
        // Below call goes to Base::executeBatch() method, and because Base class has it's "local"
        // tracer instance, which is still null, the call fails.
        child.executeBatch();
    }
    }

Proposal: May be we should change the constructor of Child class to below: [ It should NOT maintain it's own local copy of Tracer object ]

public Child(Tracer tracer) {
    super(tracer);
}

Here is the unit test case:

public void testBatchInsert() {
    Connection conn = null;
    PreparedStatement pstmt = null;
    Tracer tracer = null;

    try {
        tracer = getTracer();    // Utility method to return platform tracer object.     

        Class.forName("io.opentracing.contrib.jdbc.TracingDriver");

        // Now, try setting the tracer instance.
        for (Enumeration<Driver> enm = DriverManager.getDrivers(); enm.hasMoreElements(); ) {
            Driver driver = enm.nextElement();
            if (driver instanceof ELFTracingDriver) {
                ((TracingDriver)driver).setTracer(tracer);
            }
        }
        conn = DriverManager.getConnection("jdbc:postgresql://localhost:5432/postgres");

        pstmt = conn.prepareStatement("INSERT INTO employees (emp_id, emp_name, location, age) VALUES (?, ?, ?, ?)");

        for (int i = 1; i <= 10; i ++) { 
            pstmt.setInt(1, i);
            pstmt.setString(2, "Name-" + i);
            pstmt.setString(3, "HV-" + i);
            pstmt.setInt(4, i * 15);

            pstmt.addBatch();
        }
        int[] count = pstmt.executeBatch();    // FAILS here.
        conn.commit();
        Assert.assertEquals("Must insert two employees", 10, count.length);
    } catch (Exception e) {
       try {
            getConnection().rollback();
        } catch (SQLException ex) {
            // Do Nothing
        }
    } finally {
        close(pstmt, conn);
    }
}
malafeev commented 5 years ago

thanks @sudiptasish , got it. So my merged PR #38 should fix the problem now. Let me just release a new version.

malafeev commented 5 years ago

0.1.3 version released