Adaptive cyclic database iterator

Question

This is a bit tricky to explain. I'm trying to model a cyclic iterator that would return the next available DataSource(SQL). So infinite calls would just pick the next source nicely. The new twist comes with the idea that one DataSource might be 'busy' or somewhat. That is abstracted as a 'waiting' datasource. This designation must be set by the user. Here is my idea to implement a cyclic db source in addition to skipping specified sources for a certain amount of time.

Members:
    private Map<String,DataSource> dsMap
    private final PriorityQueue<WaitingDataSource> waitingDBList;
    private final Map<Integer, Object> waitingIndexes;
    private int index = 0;
    private Integer betweenInterval;
    private Boolean usingWaitingDB;

The next method:

 @Override
    public DataSource next() {
        index = index % dsMap.size();
        int nextIndex = hasNextDB();
        while(nextIndex != -1 && waitingIndexes.containsKey(nextIndex)) nextIndex = hasNextDB();
        if(nextIndex != -1) {
            index++;
            return dsMap.get(context.get(nextIndex));
        }
        else {
            throw new NoSuchElementException("No databases found.");
        }
    }

The alternative hasNext method:

  public int hasNextDB() {
        if(usingWaitingDB) {
            //if all databases are waiting, see if one is ready to go, or sleep
            if(waitingDBList.size() == dsMap.size()) {
                WaitingDataSource longest = waitingDBList.peek();
                if( (System.currentTimeMillis() - longest.getTimestamp()) < (betweenInterval/1000)) {
                    return -1;
                }
                else {
                    waitingDBList.remove(longest);
                    return longest.index;
                }
            }
            else {
                return index;
            }
        }
        else {
            return index;
        }
    }

Purpose of not using the interface hasNext method: In my original idea, hasNext would simply return false if there are no more datasources available. However, then I would actually have to iterate to find the next datasource if the next one is not available. Additionally, I'm using a priority queue to check if the longest waiting DB has not yet waiting long enough, so we immediately return. Since I already have that, I'm simply returning the oldest's index in the scenario that all databases are full, since we already know which one we want to return.

This is to designate a db as unavailable:

 public void addWaitingDB(DataSource ds) {
        waitingDBList.add(new WaitingDataSource(ds, System.currentTimeMillis(), index));
        waitingIndexes.put(index, placeholder);
    }

Here I'm simply keeping track of the indexes that were taken. I initially thought of using an array and just fill the index position with some value, but I didn't want to enforce a limit with the array size.

And the inner wrapper class to the DataSource:

private class WaitingDataSource {
        private final DataSource ds;
        private final long timestamp;
        private final int index;

        WaitingDataSource(DataSource ds, long timestamp, int index) {
            this.ds = ds;
            this.timestamp = timestamp;
            this.index = index;
        }

        public DataSource getDataSource() {
            return ds;
        }

        public long getTimestamp() {
            return timestamp;
        }

        public int getIndex() {
            return index;
        }
    }

The priority list comparator:

waitingDBList = new PriorityQueue<WaitingDataSource>(CONTEXT_JDBC.size(), new Comparator<WaitingDataSource>() {
            @Override
            public int compare(WaitingDataSource o1, WaitingDataSource o2) {
                if(o1.getTimestamp() < o2.getTimestamp()) {
                    return 1;
                }
                else if(o1.getTimestamp() == o2.getTimestamp()) {
                    return 0;
                }
                else {
                    return -1;
                }
            }
        });

I suppose the biggest questions I have here is - am I missing some design pattern that would make this more intuitive. Including the iterator interface seems to only make things more confusing. Additionally, I'm not confident that keeping a Map of unavailable indexes is the best course of action.

Answer 1

Purpose of not using the interface hasNext method: In my original idea, hasNext would simply return false if there are no more datasources available. However, then I would actually have to iterate to find the next datasource if the next one is not available.

This sounds wrong. You'd return false indicating that there's no next and then try to get some next otherwise.

I guess, you should really implement a consistent Iterator doing exactly what you want. The iteration order is up to you and so is the cyclic nature of the iteration.

I don't really understand you code, so take it with a grain of salt.

---

 public void addWaitingDB(DataSource ds) {
        waitingDBList.add(new WaitingDataSource(ds, System.currentTimeMillis(), index));
        waitingIndexes.put(index, placeholder);
    }

What placeholder? If you don't need it, then use a HashSet instead (which is nothing but a HashMap adapter with a placeholder).

---

Every next method should do something like this

public DataSource next() {
    if (!hasNext()) {
        throw new NoSuchElementException();
    }
    return next; // the result computed by the hasNext call
}

This makes them automatically consistent. Now all the real work is to be done in hasNext, which should look like

public boolean hasNext() {
    if (next!=null) {
        return true;
    }
    ... now do the real work and set next if found ...
}

---

This method looks like the one doing the actual work. I'd made it private and stick with the Iterator methods.

public int hasNextDB() {
    if(usingWaitingDB) {

Put a space after "if".

        //if all databases are waiting, see if one is ready to go, or sleep
        if(waitingDBList.size() == dsMap.size()) {

Again. WaitingDataSource longest = waitingDBList.peek(); if( (System.currentTimeMillis() - longest.getTimestamp()) < (betweenInterval/1000)) {

Again. And remove the other. And the useles parentheses.

                return -1;
            }
            else {

Your braces style is not Java. Anyway, with this style, I'd use the fact that the "else" is not needed because of the "return".

                waitingDBList.remove(longest);
                return longest.index;
            }
        }

The following lines are pretty overcomplicated way to say just "return index" at the end.

        else {
            return index;
        }
    }
    else {
        return index;
    }
}

Summarized, I'd go for

private int hasNextDB() {
    if (usingWaitingDB) {
        //if all databases are waiting, see if one is ready to go, or sleep
        if (waitingDBList.size() == dsMap.size()) {
            WaitingDataSource longest = waitingDBList.peek();
            if (System.currentTimeMillis() - longest.getTimestamp() < betweenInterval / 1000) {
                return -1;
            }
            waitingDBList.remove(longest);
            return longest.index;
        }
    }
    return index;
}

and use it in hasNext like

public boolean hasNext() {
    if (next!=null) {
        return true;
    }
    int nextIndex = hasNextDB();
    if (nextIndex == -1) {
        return false;
    }
    next = dsMap.get(context.get(nextIndex));
}

I guess, I missed some features (e.g. your looping in next). But I'd suggest you clean up your code a bit first, so that we can continue.

---

I suppose the biggest questions I have here is - am I missing some design pattern that would make this more intuitive.

What you're doing is filtering an iterator. With something like Guava's FluentIterable#filter (or JDK 8) this can get pretty simple.