Skip to content

Conversation

@kerneltime
Copy link
Contributor

@kerneltime kerneltime commented Jan 17, 2024

What changes were proposed in this pull request?

isKeyPresentInTable should use the constructor with prefix.

The table iterator implemented over rocksDB can seek to the prefix if provided at the time to construction. This can avoid wasteful seekToFirst behavior that is part of the construction of the iterator.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10154

How was this patch tested?

Existing tests

Change-Id: I90132387bb42e85332a1aaea924c4feab13d701e
@kerneltime kerneltime requested a review from duongkame January 17, 2024 23:44
@kerneltime kerneltime changed the title HDDS-10154: isKeyPresentInTable should use the constructor with prefix HDDS-10154. isKeyPresentInTable should use the constructor with prefix Jan 17, 2024
@errose28
Copy link
Contributor

@kerneltime can you explain the difference in the two options? Looking at the code it seems both options end up calling RDBStoreAbstractIterator#seek0 soon after the iterator is created. Based on the RocksDB API it does not look like the underlying implementation does anything different in these two cases.

@kerneltime
Copy link
Contributor Author

@kerneltime can you explain the difference in the two options? Looking at the code it seems both options end up calling RDBStoreAbstractIterator#seek0 soon after the iterator is created. Based on the RocksDB API it does not look like the underlying implementation does anything different in these two cases.

When the constructor is called without prefix

  RDBStoreByteArrayIterator(ManagedRocksIterator iterator,
      RDBTable table, byte[] prefix) {
    super(iterator, table,
        prefix == null ? null : Arrays.copyOf(prefix, prefix.length));
    seekToFirst();
  }

seekToFirst() does

  public final void seekToFirst() {
    if (prefix == null) {
      rocksDBIterator.get().seekToFirst();
    } else {
      seek0(prefix);
    }
    setCurrentEntry();
  }

which in turn calls

  @Override
  public void seekToFirst() {
    assert (isOwningHandle());
    seekToFirst0(nativeHandle_);
  }

@kerneltime
Copy link
Contributor Author

kerneltime commented Jan 19, 2024

@kerneltime can you explain the difference in the two options? Looking at the code it seems both options end up calling RDBStoreAbstractIterator#seek0 soon after the iterator is created. Based on the RocksDB API it does not look like the underlying implementation does anything different in these two cases.

This behavior was first fixed for https://issues.apache.org/jira/browse/HDDS-8289. There needs to be a clean up of TableIterators to behave the same way when in memory vs via RocksDB. My initial attempt to clean up this code did not go as cleanly as this works only when the table type is RocksDB. I am continuing to investigate how to make the behavior the same (always initialize the iterator with prefix when prefix is available) independent of the backing table.

Copy link
Contributor

@duongkame duongkame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch @kerneltime . LGTM.

@adoroszlai adoroszlai merged commit 6f6ec58 into apache:master Jan 19, 2024
@adoroszlai
Copy link
Contributor

Thanks @kerneltime for the patch, @duongkame, @errose28 for the review.

Tejaskriya pushed a commit to Tejaskriya/ozone that referenced this pull request Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants