Skip to content
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

Ticker and Histogram enumeration types out of sync with Java API #2171

Closed
bentorfs opened this issue Apr 18, 2017 · 8 comments
Closed

Ticker and Histogram enumeration types out of sync with Java API #2171

bentorfs opened this issue Apr 18, 2017 · 8 comments

Comments

@bentorfs
Copy link
Contributor

Problem: The enumerations of tickers and histogram types for retrieving statistics are not in sync between the C++ source and the java API:

Java version:
https:/facebook/rocksdb/blob/master/java/src/main/java/org/rocksdb/TickerType.java
https:/facebook/rocksdb/blob/master/java/src/main/java/org/rocksdb/HistogramType.java
C++ version:
https:/facebook/rocksdb/blob/master/include/rocksdb/statistics.h

The java Enumeration is missing a lot of declarations, and their values do not correspond to the (implicit) order of values in the C++ enum.

Impact: for example, retrieving the BLOOM_FILTER_USEFUL statistic through the java API, actually returns the data for BLOCK_CACHE_FILTER_MISS.

@lightmark
Copy link
Contributor

Yes. Since we are short of ppl maintaining Java api, would you like to contribute?

@koldat
Copy link
Contributor

koldat commented May 2, 2017

@adamretter I was facing same issue. In this case I would recommend to use portable (maybe slower) way (strings over numbers). I made a change in this way in our local repo using TickersNameMap. It works perfectly and does not require to maintain the enumerations at all. What do you think?

@adamretter
Copy link
Collaborator

@koldat Is it possible to see an example?

@koldat
Copy link
Contributor

koldat commented May 2, 2017

@adamretter Roughly like this:

Statistics.java:

  private static ConcurrentMap<String, Integer> tickerCache = new ConcurrentHashMap<>();
  
  // Returns -1 is mapping not found
  private native int getTickerId0(String tickerType, long handle);
  
  // Returns -1 is mapping not found
  public long getTickerCount(String tickerType) {
    assert(isInitialized());
      
    Integer type = tickerCache.get(tickerType);
    if (type == null) {
      type = getTickerId0(tickerType, statsHandle_);
      tickerCache.put(tickerType, type);
    }
    if (type == -1) {
        return -1;
    }
      
    return getTickerCount0(type, statsHandle_);
  }

// The same for histograms

And enumeration can reference strings. For missing enum one can still use string without library change.

@adamretter
Copy link
Collaborator

@koldat Sorry I can't understand what you mean from the above code, I don't see any enums involved there.

@koldat
Copy link
Contributor

koldat commented May 2, 2017

@adamretter That is actual usage. Enum can be:

public enum TickerType {
  // total block cache misses
  // REQUIRES: BLOCK_CACHE_MISS == BLOCK_CACHE_INDEX_MISS +
  //                               BLOCK_CACHE_FILTER_MISS +
  //                               BLOCK_CACHE_DATA_MISS;
  BLOCK_CACHE_MISS("rocksdb.block.cache.miss");
  // total block cache hit
  // REQUIRES: BLOCK_CACHE_HIT == BLOCK_CACHE_INDEX_HIT +
  //                              BLOCK_CACHE_FILTER_HIT +
  //                              BLOCK_CACHE_DATA_HIT;

  private final String value_;

  private TickerType(String value) {
    value_ = value;
  }

  public String getValue() {
    return value_;
  }
}

There are more use cases:

  1. C++ untroduces new ticker not maintained in enum. You can still query for it.
  2. You can use bulk query to retireve all tickers as map<String, Long> values. Regardless on enum. We have for example monitoring solution that consume string -> value map.

@koldat
Copy link
Contributor

koldat commented May 3, 2017

@adamretter What do you think? Should I go ahead and create pull request?

@gfosco
Copy link
Contributor

gfosco commented Jan 10, 2018

Closing this via automation due to lack of activity. If discussion is still needed here, please re-open or create a new/updated issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants