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

Add Filter::CreateFromString to Java #387

Merged
merged 6 commits into from
Apr 19, 2023

Conversation

mrambacher
Copy link
Contributor

This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.

@mrambacher mrambacher requested a review from udi-speedb February 7, 2023 00:22
@mrambacher mrambacher self-assigned this Feb 7, 2023
@udi-speedb
Copy link
Contributor

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

@mrambacher
Copy link
Contributor Author

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

@udi-speedb
Copy link
Contributor

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@udi-speedb
Copy link
Contributor

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@mrambacher - You haven't answered my question. Please do

ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, status);
return kStatusError;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The function returns, in case of errors, sometimes negative error codes instead of valid pointers.
Is it OK because an exception is thrown and, therefore, the status is never actually returned (just a way to let the Java code "compile")?
It's confusing IMO. Why not return a nullptr converted to jlong or 0 (as in Java_org_rocksdb_BackupEngineOptions_newBackupEngineOptions) to show the reader that it's a null pointer?
You can also document the reason for returning the "null pointer" (the reason for the error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function returns, in case of errors, sometimes negative error codes instead of valid pointers. Is it OK because an exception is thrown and, therefore, the status is never actually returned (just a way to let the Java code "compile")? It's confusing IMO. Why not return a nullptr converted to jlong or 0 (as in Java_org_rocksdb_BackupEngineOptions_newBackupEngineOptions) to show the reader that it's a null pointer? You can also document the reason for returning the "null pointer" (the reason for the error).

I was following the example used by the DB code where a Status DB::Put() would throw an exception based on the Status code. If there is a different example that makes more sense, I am willing to follow it instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just gave one in my comment above: Java_org_rocksdb_BackupEngine_open
And others: Java_org_rocksdb_BackupEngine_getCorruptedBackups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think it matters since these values should not be seen from the exceptions. If you prefer they all return 0 rather than a negative number, I will make that change.

Copy link
Contributor

@udi-speedb udi-speedb Mar 21, 2023

Choose a reason for hiding this comment

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

I do. Thanks. I think it makes the code clearer.

cfg_opts.ignore_unknown_options = false;
cfg_opts.invoke_prepare_options = true;
return createSharedFromString<R, T>(cfg_opts, env, s);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These fields have defaults on the cpp side. Why override them here as you do? Aren't those defaults (the cpp ones defined for ConfigOptions) adequate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still do not understand why do you need to explicitly override the cpp defaults:

  // When true, any unused options will be ignored and OK will be returned
  bool ignore_unknown_options = false;

  // When true, any unsupported options will be ignored and OK will be returned
  bool ignore_unsupported_options = true;

  // If the strings are escaped (old-style?)
  bool input_strings_escaped = true;

  // Whether or not to invoke PrepareOptions after configure is called.
  bool invoke_prepare_options = true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only value that is different than the default is "ignore_unsupported_options=false". The rest match the default. This value would be nice to default to false, but would cause a lot of old code to fail. I could not make "unsupported=false" for historical reasons; since Java does not have this history, we can turn this off here.

Copy link
Contributor

@udi-speedb udi-speedb Mar 21, 2023

Choose a reason for hiding this comment

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

I think it's best to have the JAVA default be the same as the CPP defaults. That way JAVA users get the same behaviour as cpp users by default.
Shouldn't you just avoid overriding the defaults at all (and have the defaults be defaults)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@udi-speedb I can change it so the defaults match the C++. However, I am not certain that is what is desired in this case. The C++ ones are what they are to support older settings. For example, the default for "ignore_unsupported_options=true" would mean that attempting to create a filter that does not exist may return OK when it should return an error. This is a historical artifact as C++ behaved this way before the new infrastructure was added and needs to for compatibility. Java -- having no such compatibility issues, since this is new code -- can return errors properly under those conditions.

If you still feel strongly about it, I can make the defaults match. However, I do not know I agree that is the right thing to do.

public void createUnknownFromString() throws RocksDBException {
try (final Filter filter = Filter.createFromString("unknown")) {
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test to create our filter as well. This is both expected IMO (as this is our new type) and will also test our ability to create a plugin using the JNI.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrambacher - Could you please somehow address this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrambacher - Could you please somehow address this comment?

@udi-speedb There is a test added under plugin/speedb/java for the Speedb-specific test.

ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, status);
return kStatusError;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I just gave one in my comment above: Java_org_rocksdb_BackupEngine_open
And others: Java_org_rocksdb_BackupEngine_getCorruptedBackups

cfg_opts.ignore_unknown_options = false;
cfg_opts.invoke_prepare_options = true;
return createSharedFromString<R, T>(cfg_opts, env, s);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I still do not understand why do you need to explicitly override the cpp defaults:

  // When true, any unused options will be ignored and OK will be returned
  bool ignore_unknown_options = false;

  // When true, any unsupported options will be ignored and OK will be returned
  bool ignore_unsupported_options = true;

  // If the strings are escaped (old-style?)
  bool input_strings_escaped = true;

  // Whether or not to invoke PrepareOptions after configure is called.
  bool invoke_prepare_options = true;

@udi-speedb
Copy link
Contributor

@mrambacher - I have replied on the updates. Please note that there are still some comments that you haven't addressed.
Thanks

@mrambacher
Copy link
Contributor Author

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@mrambacher - You haven't answered my question. Please do

How else would you like this tested? I do not see where the the other filter code is tested in any more depth than this API.

@udi-speedb udi-speedb self-requested a review March 21, 2023 02:58
@udi-speedb
Copy link
Contributor

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@mrambacher - You haven't answered my question. Please do

@mrambacher - Could you please answer

@udi-speedb
Copy link
Contributor

@mrambacher - Still some comments / questions . Thanks

@Yuval-Ariel
Copy link
Contributor

@mrambacher , this can be merged but please squash the commits when you do and provide a descriptive commit msg

@mrambacher
Copy link
Contributor Author

@mrambacher , this can be merged but please squash the commits when you do and provide a descriptive commit msg

@Yuval-Ariel I have no idea how to do a squash since my branch also contains updates merged from main. Is there a reason "squash and merge" is insufficient?

@Yuval-Ariel Yuval-Ariel merged commit 5817370 into speedb-io:main Apr 19, 2023
Yuval-Ariel pushed a commit that referenced this pull request May 2, 2023
This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.
Yuval-Ariel pushed a commit that referenced this pull request May 4, 2023
This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.
udi-speedb pushed a commit that referenced this pull request Nov 13, 2023
This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.
udi-speedb pushed a commit that referenced this pull request Nov 15, 2023
This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.
udi-speedb pushed a commit that referenced this pull request Dec 5, 2023
This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Upstreamable can be upstreamed to RocksDB usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose the new Paired bloom filter via the Java APIs
3 participants