Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on a project in which I have two tables in a different database with different schemas. So that means I have two different connection parameters for those two tables to connect using JDBC-

Let's suppose below is the config.property file-

TABLES: table1 table2

#For Table1
table1.url: jdbc:mysql://localhost:3306/garden
table1.user: gardener
table1.password: shavel
table1.driver: jdbc-driver
table1.percentage: 80

#For Table2
table2.url: jdbc:mysql://otherhost:3306/forest
table2.user: forester
table2.password: axe
table2.driver: jdbc-driver
table2.percentage: 20

Below method will read the above config.properties file and make a ReadTableConnectionInfo object for each tables.

  private static void readPropertyFile() throws IOException {

    prop.load(Read.class.getClassLoader().getResourceAsStream("config.properties"));

    tableNames = Arrays.asList(prop.getProperty("TABLES").split(" "));

    for (String arg : tableNames) {

        ReadTableConnectionInfo ci = new ReadTableConnectionInfo();

        String url = prop.getProperty(arg + ".url");
        String user = prop.getProperty(arg + ".user");
        String password = prop.getProperty(arg + ".password");
        String driver = prop.getProperty(arg + ".driver");
        double percentage = Double.parseDouble(prop.getProperty(arg + ".percentage"));

       ci.setUrl(url);
       ci.setUser(user);
       ci.setPassword(password);
       ci.setDriver(driver);
       ci.setPercentage(percentage);

        tableList.put(arg, ci);

    }
}

Below is the ReadTableConnectionInfo class that will hold all the table connection info for a particular table.

public class ReadTableConnectionInfo {

    public String url;
    public String user;
    public String password;
    public String driver;
    public String percentage;

    public String getUrl() {
        return url;
    }

    public void setUrl(String url) {
        this.url = url;
    }

    public String getUser() {
        return user;
    }

    public void setUser(String user) {
        this.user = user;
    }

    public String getPassword() {
        return password;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    public String getDriver() {
        return driver;
    }

    public void setDriver(String driver) {
        this.driver = driver;
    }
}

Now I am creating ExecutorService for specified number of threads and passing this tableList object (that I created by reading the config.property file) to constructor of ReadTask class -

// create thread pool with given size
ExecutorService service = Executors.newFixedThreadPool(10);

for (int i = 0; i < 10; i++) {
    service.submit(new ReadTask(tableList));
}

Below is my ReadTask class that implements Runnable interface in which each thread will make two connections for each table in the starting before doing anything meaningful.

class ReadTask implements Runnable {

    private Connection[] dbConnection = null;
    private ConcurrentHashMap<ReadTableConnectionInfo, Connection> tableStatement = new ConcurrentHashMap<ReadTableConnectionInfo, Connection>();
private PreparedStatement preparedStatement = null;
private ResultSet rs = null;

private static Random random = new SecureRandom();

    public ReadTask(LinkedHashMap<String, ReadTableConnectionInfo> tableList) {
        this.tableLists = tableList;
    }


    @Override
    public void run() {

        try {

            int j = 0;
            dbConnection = new Connection[tableList.size()];

            //loop around the map values and make the connection list
            for (ReadTableConnectionInfo ci : tableList.values()) {

                dbConnection[j] = getDBConnection(ci.getUrl(), ci.getUser(), ci.getPassword(), ci.getDriver());
                tableStatement.putIfAbsent(ci, dbConnection[j]);

                j++;
        }

long startTime = System.currentTimeMillis();
long endTime = startTime + (10 * 60 * 1000);

while (System.currentTimeMillis() <= endTime) {


    double randomNumber = random.nextDouble() * 100.0;
    ReadTableConnectionInfo table = selectRandomConnection(randomNumber);

    for (Map.Entry<ReadTableConnectionInfo, Connection> entry : tableStatement.entrySet()) {

        //The below if condition will be fine from ThreadSafety issues?
        if (entry.getKey().getTableName().equals(table.getTableName())) {

            final String id = generateRandomId(random);
            final String selectSql = generateRandomSQL(table);

            preparedStatement = entry.getValue().prepareCall(selectSql);
            preparedStatement.setString(1, id);

            rs = preparedStatement.executeQuery();
        }
            }
        }
        }
     }

private String generateRandomSQL(ReadTableConnectionInfo table) {

    int rNumber = random.nextInt(table.getColumns().size());

    List<String> shuffledColumns = new ArrayList<String>(table.getColumns());
    Collections.shuffle(shuffledColumns);

    String columnsList = "";

    for (int i = 0; i < rNumber; i++) {
        columnsList += ("," + shuffledColumns.get(i));
    }

    final String sql = "SELECT ID" + columnsList + "  from "
            + table.getTableName() + " where id = ?";

    return sql;
}


private ReadTableConnectionInfo selectRandomConnection(double randomNumber) {

    double limit = 0;
    for (ReadTableConnectionInfo ci : tableLists.values()) {
        limit += ci.getPercentage();
        if (random.nextDouble() < limit) {
            return ci;
        }
        throw new IllegalStateException();
    }
    return null;
}

 }

private Connection getDBConnection(String url, String username, String password, String driver) {

    Connection dbConnection = null;

    try {
        Class.forName(driver);

        dbConnection = DriverManager.getConnection(url, username, password);
    }

    return dbConnection;
}

In my above try block, I am making dbConnection array for storing two different database connections. And then I have a tableStatement as ConcurrentHashMap in which I am storing ReadTableConnectionInfo object with it's dbConnection. For example Table1 object will have Table1 connection in the tableStatement ConcurrentHashMap.

And then after that I am applying this logic-

/* Generate random number and check to see whether that random number
 * falls between 1 and 80, if yes, then choose table1
 * and then use table1 connection and statement that I made above and do a SELECT * on that table.
 * If that random numbers falls between 81 and 100 then choose table2 
 * and then use table2 connection and statement and do a SELECT * on that table
 */

1) Generating Random number between 1 and 100.
2) If that random number is less than table1.getPercentage() then I am choosing `table1` and then use `table1 connection` object to make a SELECT sql call to that database.
else choose `table2` and then use `table2 connection object` to make a SELECT sql call to that database.

Problem Statement:-

I am trying to see whether is there any potential thread safety issues here in my run method?

share|improve this question
1  
According to the FAQ the code should work but I'm afraid this one does not compile (preparedStatement is not declared anywhere, while (System.currentTimeMillis() <= 60 minutes) is an invalid expression). –  palacsint Feb 26 '13 at 8:11
1  
Nor do you show where tableList comes from - considering it is the only shared object in your code (unless preparedStatement or rs also are, but you don't show how they are declared either), this is clearly a critical piece of information. You also need to show how getDBConnection is implemented. –  assylias Feb 26 '13 at 12:24
1  
If you are using a .properties file, then it should be in the proper format (key-value pairs separated by =, not :). –  E-Man Feb 26 '13 at 14:00
1  
@palacsint, That one I removed it intentionally, as it was straight forward. Anyways, I have added that chunk piece of code. Please take a look now and let me know if everything looks good. –  user21973 Feb 26 '13 at 16:11
1  
@assylias, I have updated my code with getDBConnection method and added preparedStatement and rs as well. Regarding tableList, it is coming from the readProperty method right? and them I am passing it to ExecutorService. Please take a look now and let me know. –  user21973 Feb 26 '13 at 16:13
add comment

1 Answer

In summary, you have 2 shared resources: the tableList variable and the database.

Regarding tableList, if:

  • tableList is fully populated before you submit the tasks to the executor service AND
  • it is populated in the same thread as the main thread that creates the executor and submits the tasks AND
  • it is not modified after that

THEN the access in read-only mode to that shared variable is thread-safe (thanks to the semantics of the submit method which creates a happens-before relationship.

Actions in a thread prior to the submission of a Runnable or Callable task to an ExecutorService happen-before any actions taken by that task.

Regarding the access to the database, DriverManager.getConnection() is thread safe and you don't share the connection object across threads so you are fine (although it would probably be more efficient to use a connection pool).

So your code looks thread safe to me.

share|improve this answer
    
Thanks assylias for the suggestion. What about this if loop if (entry.getKey().getTableName().equals(table.getTableName())) { that I have in my while block? Is there any problem with that? –  user21973 Feb 26 '13 at 16:55
    
tableStatement is "thread local" (it is only accessed from one thread) - by the way you don't really need a concurrent map there for the same reason. And I suppose that getTableName(), which is not defined in your code, is a method of a ReadTableConnectionInfo, which is not modified once it is passed to your thread. So I don't see why there would be a problem there. –  assylias Feb 26 '13 at 17:09
    
Thanks. That make more sense. Yes it is defined in ReadTableConnectionInfo class. And about why I am using ConcurrentHashMap is because- In the starting for loop of my run method, I am putting the data in Map. So I believe putting any data in HashMap is not thread safe so that is the reason I am using ConcurrentHashMap here. let me know if I am missing anything. –  user21973 Feb 26 '13 at 17:52
    
@user21973 You populate and read the map from the same thread and no other thread has access to the map, so there is no thread safety concern. If the map were static and shared across your Runnables, then yes you would need to synchronize access. –  assylias Feb 26 '13 at 18:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.