JAVA-3030 Use configuration to determine batch-type instead of settin… #1619

Maxwell-Guo · 2022-11-16T06:38:56Z

JAVA-3030 Use configuration to determine batch-type instead of setting on a per-batch transaction basis

…g on a per-batch transaction basis

absurdfarce

@Maxwell-Guo This is really good work! There are some things I think we need to change (see my comments for more detail) but please don't let this discourage you; I really like what you're working on here. The quantity and nature of your tests alone is a great sign!

There are three areas of change I'd like to see from what you have here:

Make sure the relevant config for default batch type is specified in reference.conf, ideally with some comments explaining what it is and how to use it. There are other examples in some of the other entries in reference.conf
Remove the UNKNOWN batch type and make sure that we always have a batch type specified at BatchStatement construction type. This also enables the removal of the null batch type check in CqlRequestHandler.
Remove the batch type registry all together

If you have any questions about any of this please don't hesitate to ask!

absurdfarce · 2022-11-16T06:43:53Z

core/src/main/java/com/datastax/oss/driver/api/core/cql/DefaultBatchType.java

+ /** UNKNOW BATCH TYPE when user do not set at BatchStatement builder */
+ // TODO should we set the number 3 be a constant at some where or ProtocolConstants.BatchType add
+ // a unknow type ?
+ UNKNOW((byte) 3),


Items in the DefaultBatchType represent the set of batch types supported by the driver by default... which should be equal to what's defined in the v5 protocol spec. If you wanted to define a custom batch type here for some reason then this should be done in com.datastax.oss.driver.api.core.cql.BatchType only.

That said, it's far from clear to me that we need to do such a thing here.

I have also define a BatchType in com.datastax.oss.driver.api.core.cql.BatchType and the type is named UNKNOW and the value of https://github.com/Maxwell-Guo/java-driver/blob/4.x-JAVA-3030/core/src/main/java/com/datastax/oss/driver/api/core/cql/BatchType.java#L31

The reason that an UNKNOW BatchType is defined is that after I read the jira and the example of how BatchStatement is used for the users in the codebase's tests , I found there is three ways :

BatchStatement.builder with the Batch Type setted;

BatchStatement.newInstance with the Batch Type setted ;

Just set the cql string for SimpleStatement ,this way if batch type is not write in cql string the c* will use LOGGED type;

So I add a BatchStatement.builder which no BatchType is needed for the input parameter, and then a flag named UNKNOW batch type is setted for this kind of BatchStatement, then I can use this flag to identify whether the user actively sets the batchtype. what I want to do is that when user do not set the batch type on batch invocation ,the configured batch type will be used ; when user set the batch type on batch invocation ,the user setted type is used.

In fact, at the beginning of this jira, we should discuss the how user use the function that the jira will support. Or is the useage
(https://github.com/Maxwell-Guo/java-driver/blob/4.x-JAVA-3030/integration-tests/src/test/java/com/datastax/oss/driver/core/cql/BatchStatementUseConfigIT.java#L82 defained) that user need ?

As we discuss below I think you'll find it much easier to just enforce a batch type at BatchStatement creation... you just need some way to allow users to say "I want to use the batch type specified in my config". That should address this issue (unless I'm missing something else).

absurdfarce · 2022-11-16T06:44:13Z

core/src/main/java/com/datastax/oss/driver/internal/core/BatchTypeRegistry.java

+
+ /** @return all the values known to this driver instance. */
+ Iterable<BatchType> getValues();
+}


This is the kind of change that sets off warning bells for me. There needs to be a pretty high threshold for adding new items to the DriverContext and it's far from clear to me that the need for a registry for batch types meets that threshold.

absurdfarce · 2022-11-16T06:44:40Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/Conversions.java

+ return batchStatement.setBatchType(batchType);
+ }
+ }
+ return statement;


Probably makes sense to switch the order around here; if the statement isn't a batch statement just return, otherwise do the logic above. In most cases batch statements will likely not be the majority of statements executed so exiting quickly for non-batches seems desirable.

absurdfarce · 2022-11-16T06:45:11Z

integration-tests/src/test/resources/application.conf

 init-query-timeout = 5 seconds
 set-keyspace-timeout = 5 seconds
 }
+ batch-type-configuration = UNLOGGED


This covers the config for integration testing only. We also should have an entry (ideally with documentation) in reference.conf as well.

for i'm not familiar with the codebase , so the first patch may be just a test with a begining. I will modify this in reference.conf with some detail documention.

absurdfarce · 2022-11-16T07:05:31Z

core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlRequestHandler.java

 this.session = session;
 this.keyspace = session.getKeyspace().orElse(null);
 this.context = context;
+ this.initialStatement = Conversions.resolveStatement(statement, context);


I agree that this is one way to address the problem: before you send a batch statement make sure that you've got a batch type. But this has a couple of very unpleasant consequences:

You have to introduce something like an "unknown" batch type to represent the state of batches which haven't had anything specified yet because they're going to use the default at execution time.

In driver 4.x statements are immutable, so every time you have to use the default batch type you're creating a copy of the input BatchStatement object with exactly one field changed.

The much simpler answer is to enforce the presence of a batch type at creation time. The builder interface at BatchStatement.builder() already requires a non-null BatchType so all we need to worry about are the BatchStatement.newInstance() types. In that case it seems like we can make a modified version which accepts a non-null DriverContext as a param instead of a BatchType. We can then use that DriverContext to do a lookup for the configured default batch type. So long as our config defines a default (which it certainly should) we'll always generate a BatchStatement with a non-null batch type using that method.

Sorry for Not familiar with the codebase , and I have got this one question here ;do you mean something like : BatchStatement.newInstance(DriverContext context)? It seems DriverContext can ony be get from the variable of session, so what kind of usage of the interface can user use ? That is what I am asked at the beigining , is https://github.com/Maxwell-Guo/java-driver/blob/4.x-JAVA-3030/integration-tests/src/test/java/com/datastax/oss/driver/core/cql/BatchStatementUseConfigIT.java#L82 people need ? or something eles.

Yes, BatchStatement.newInstance(DriverContext ctx) is exactly what I had in mind. Keep in mind that the user will have to establish a Session of some kind in order to execute any of these statements. The "Quick start" example in the manual demonstrates a really simple example. For batch statements the user would be creating a BatchStatement object and then adding other statement objects to it but you can see how the general API works here.

The upshot is that being able to extract a DriverContext from the Session shouldn't be a problem when the user goes about creating a BatchStatement instance.

absurdfarce · 2022-11-16T07:08:13Z

core/src/main/java/com/datastax/dse/driver/api/core/config/DseDriverOption.java

+ *
+ * <p>Value-type: {@link String}
+ */
+ BATCH_TYPE_CONFIGURATION("advanced.batch-type-configuration"),


This enum represents the set of config values for options specific to DSE. We want this config to be available for Cassandra users as well so it really should be in com.datastax.oss.driver.api.core.config.DefaultDriverOption instead.

at first , I just put this enum variable in DefaultDriverOption for I think it should also be used by c* users, But after I read the jira agagin I found out that The description of the jira is "FedEx (David Twynam) would like the ability to set batch-type application-wide ", I think FedEx may be one of datastax's customers ，then I change the BATCH_TYPE_CONFIGURATION to DseDriverOption , I will change it back to DefaultDriverOption.

absurdfarce · 2022-11-16T07:10:03Z

core/src/main/java/com/datastax/oss/driver/api/core/cql/BatchStatement.java

+ */
+ static BatchStatementBuilder builder() {
+ return new BatchStatementBuilder(BatchType.UNKNOW);
+ }


I don't think we want to do this at all. I mentioned this in comments below but the current builder interface on this class already requires users to provide a non-null BatchType in order to get a builder. That seems like exactly the kind of thing we want to encourage. We should only worry about fixing the newInstance() methods above, and I provided some feedback on a way to do that that doesn't require the introduction of an UNKNOWN batch type.

Thanks , I aggree that introduce an UNKNOW type is not a good idea.

JAVA-3030 Use configuration to determine batch-type instead of settin…

ee742f6

…g on a per-batch transaction basis

absurdfarce self-requested a review November 16, 2022 06:43

absurdfarce requested changes Nov 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JAVA-3030 Use configuration to determine batch-type instead of settin… #1619

JAVA-3030 Use configuration to determine batch-type instead of settin… #1619

Maxwell-Guo commented Nov 16, 2022

absurdfarce left a comment •

edited

Loading

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

absurdfarce Dec 2, 2022

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

absurdfarce Dec 2, 2022

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

absurdfarce Nov 16, 2022

Maxwell-Guo Nov 16, 2022

Labels

2 participants

JAVA-3030 Use configuration to determine batch-type instead of settin… #1619

Are you sure you want to change the base?

JAVA-3030 Use configuration to determine batch-type instead of settin… #1619

Conversation

Maxwell-Guo commented Nov 16, 2022

absurdfarce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

2 participants

absurdfarce left a comment •

edited

Loading