Simplify JDBC code sample #4855
Conversation
|
This change is |
|
@jordanlewis can you please help me find someone on our engineering team who has Java expertise and would be willing to review this PR for:
I am expecting that the code and possibly the doc surrounding it will need updates according to the criteria above, hence the WIPness. My initial plan was to ask around but I spoke to @awoods187 and he recommended coming directly to you as a team lead / engineering manager type person. Happy to discuss offline if that's easier/quicker. |
|
Hey @rmloveland, will follow up with you offline. Thanks for bringing this up! |
|
Hey @rafiss, could you please take a look at this code sample while your Java knowledge is still fresh? The context is that we're trying to improve our docs for Java users in particular. We'd like to show a canonical way of interacting with the database. Your review would be valuable, as you have recent Java experience. |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rmloveland)
_includes/v19.1/app/BasicExample.java, line 20 at r1 (raw file):
// 1. BasicExample, which is where the application logic lives // 2. BasicExampleDAO, which is used by the application to access the // data store
this comment could also be a part of the multi-line comment above
_includes/v19.1/app/BasicExample.java, line 76 at r1 (raw file):
} class BasicExampleDAO {
normally this would be a public class in a separate BasicExampleDAO.java file, but if this is just done in-line for the sake of brevity it seems fine.
consider adding a javadoc style comment with
/**
* description goes here
*/
_includes/v19.1/app/BasicExample.java, line 83 at r1 (raw file):
private static final boolean FORCE_RETRY = false; private final DataSource ds;
this suggestion might be besides the point, but i think it would be quite rare for anyone to use the DataSource directly in the real world. it would be more common to have a library like JDBI that handles the execution/binding/mapping.
but of course that adds another dependency, so if the goal is just to use standard java stuff, then this seems fine
_includes/v19.1/app/BasicExample.java, line 93 at r1 (raw file):
// this calls an internal CockroachDB function that can only be // run by the 'root' user, and will fail with an insufficient // privileges error if you try to run it as user 'maxroach'.
the method level comments should likely all be javadoc /** ... */ instead of //
for the methods that take parameters, that also lets you document them like so:
/**
* description of the method . . .
* @param paramName description of the param
* @return description of return value if any
*/
_includes/v19.1/app/BasicExample.java, line 140 at r1 (raw file):
// (a.k.a. strings). int place = 1; // Prepared statement uses 1-based indexing. for (String arg : args) {
i think it would be slightly more idiomatic here to do a for (int i ...) loop here and then have int place = i + 1 as the first line, so that you don't need the separate initializer and increment lines for place. up to your preference though
_includes/v19.1/app/BasicExample.java, line 142 at r1 (raw file):
for (String arg : args) { if (Integer.parseInt(arg) > 0) {
what if the integer is negative?
if the goal was to detect non-integers, then that should be done with a try-catch that handles NumberFormatException instead:
try {
int val = Integer.parseInt(arg);
pstmt.setInt(place, val);
catch (NumberFormatException e) {
pstmt.setString(place, arg);
}
_includes/v19.1/app/BasicExample.java, line 148 at r1 (raw file):
place++; } else {
general style convention is that the else should be on the same line as the } here and in other places, but it might not be worth worrying about style too much for this example? up to you
_includes/v19.1/app/BasicExample.java, line 150 at r1 (raw file):
else { pstmt.setString(place, arg); place++;
if you keep the for (String arg : args) form of the loop, then place++ can be pulled outside of the if-else
_includes/v19.1/app/BasicExample.java, line 155 at r1 (raw file):
if (pstmt.execute()) { final ResultSet rs = pstmt.getResultSet();
maybe worth adding a comment saying that we know that getResultSet will not return null if pstmt.execute() was true
_includes/v19.1/app/BasicExample.java, line 156 at r1 (raw file):
if (pstmt.execute()) { final ResultSet rs = pstmt.getResultSet(); ResultSetMetaData rsmeta = rs.getMetaData();
is it worth being consistent about making local variables final whenever possible? it doesn't seem like a big deal to me, but i think my natural approach would have just been to not make any of the local variables final
_includes/v19.1/app/BasicExample.java, line 170 at r1 (raw file):
// values (technically 64-bit INT8s, // the CockroachDB default). This // code could be made into a switch
the line lengths of the comment could be longer
_includes/v19.1/app/BasicExample.java, line 205 at r1 (raw file):
} connection.setAutoCommit(true);
i am not sure if this is necessary. do you know if we need it for sure?
_includes/v19.1/app/BasicExample.java, line 244 at r1 (raw file):
int fromBalance = getAccountBalance(fromId); int toBalance = getAccountBalance(toId);
technically speaking, shouldn't both of these selects and the UPSERT below all be part of the same transaction? if this example isn't concerned with that level of concurrent access it's probably fine to leave, but maybe worth adding a comment explaining that
_includes/v19.1/app/BasicExample.java, line 260 at r1 (raw file):
// We skip using the retry logic in 'runSQL()' here for the following reasons: // 1. Since this is a read (SELECT), we don't expect conflicts // 2. We need to return the balance as an integer
if reusing is desired, then runSQL could be modified to return an Integer for the result (and null if it was an update statement or not found). then this method could be implemented by
return runSQL("SELECT balance FROM accounts WHERE id = ?", id)
with a null-check if you want to be extra safe
(if you do that, then the printing logic that is currently in runSQL should probably be pulled out into the calling methods)
separately -- method implementation details might belong better as a comment inside the method rather than on the method
_includes/v19.1/app/BasicExample.java, line 262 at r1 (raw file):
// 2. We need to return the balance as an integer public int getAccountBalance(int id) { String sid = Integer.toString(id);
sid seems unused
|
Sure thing! Some of the suggestions I had might not really be worth doing. Some of the idioms or conventions I suggested probably are not important for this simple example. |
|
Thanks for the review @rafiss! Working on making these changes now. |
rmloveland
force-pushed
the
20190528-simplify-jdbc-example
branch
from
70ecb33 to
e3fcc9e
Compare
There was a problem hiding this comment.
Rafi I've made almost all of the changes you requested. In a few places I asked for clarification (esp. since I am not really a good Java programmer). Please take a look and let me know what you think - and thanks again for the review!
Reviewable status:
_includes/v19.1/app/BasicExample.java, line 20 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this comment could also be a part of the multi-line comment above
Fixed.
_includes/v19.1/app/BasicExample.java, line 76 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
normally this would be a public class in a separate BasicExampleDAO.java file, but if this is just done in-line for the sake of brevity it seems fine.
consider adding a javadoc style comment with
/** * description goes here */ </blockquote></details> Added javadoc with description of the class and pointer to a couple of maybe interesting methods. ___ *[_includes/v19.1/app/BasicExample.java, line 83 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgEDd8yAfsuyGiPbh3L:-LgdKdSX28jufg_0T4Qp:b-oob83u) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L83)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> this suggestion might be besides the point, but i think it would be quite rare for anyone to use the DataSource directly in the real world. it would be more common to have a library like [JDBI](http://jdbi.org/#_fluent_api) that handles the execution/binding/mapping. but of course that adds another dependency, so if the goal is just to use standard java stuff, then this seems fine </blockquote></details> JDBI does look nice! I think the goal here is to use standard Java stuff, but hopefully it's easy for a Java developer to translate into something else if they need (?). Based on the way our docs/etc. are laid out currently, I think if we did add some instructions for JDBI it would be in a separate doc [like we have for Hibernate](https://www.cockroachlabs.com/docs/stable/build-a-java-app-with-cockroachdb-hibernate.html) ___ *[_includes/v19.1/app/BasicExample.java, line 93 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgE7qEz4CRUGVsA6RRU:-LgdF3nr9RWzKOasmneC:b-vybwax) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L93)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> the method level comments should likely all be javadoc `/** ... */` instead of `//` for the methods that take parameters, that also lets you document them like so:/**
- description of the method . . .
- @param paramName description of the param
- @return description of return value if any
*/</blockquote></details> Changed all the method comments to javadoc, and added instructions in comment block at top of file for building the javadoc ___ *[_includes/v19.1/app/BasicExample.java, line 140 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgE6Qjr5kptWk7F_JXl:-LgdF6vzA_UmdDARxx9w:bd6qf5j) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L140)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> i think it would be slightly more idiomatic here to do a `for (int i ...)` loop here and then have `int place = i + 1` as the first line, so that you don't need the separate initializer and increment lines for `place`. up to your preference though </blockquote></details> Fixed by updating as suggested. ___ *[_includes/v19.1/app/BasicExample.java, line 142 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgE8sKNDq1WmfHPNiyn:-LgdGXwo1M_z1phA8v3d:b94jzny) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L142)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> what if the integer is negative? if the goal was to detect non-integers, then that should be done with a `try-catch` that handles `NumberFormatException` instead:try {
int val = Integer.parseInt(arg);
pstmt.setInt(place, val);
catch (NumberFormatException e) {
pstmt.setString(place, arg);
}</blockquote></details> Updated, thanks! ___ *[_includes/v19.1/app/BasicExample.java, line 148 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgE9k1e1ygjELKpuU-h:-LgdGZiL7es1XIuh3kDt:b-v4ofy2) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L148)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> general style convention is that the `else` should be on the same line as the `}` here and in other places, but it might not be worth worrying about style too much for this example? up to you </blockquote></details> Updated everywhere to use `} else {` (aka "cuddled elses") ___ *[_includes/v19.1/app/BasicExample.java, line 150 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgE72ua6znbQbKVt2kK:-LgdHEmGBPYbKT39VdXs:b-83euwu) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L150)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> if you keep the `for (String arg : args)` form of the loop, then `place++` can be pulled outside of the if-else </blockquote></details> Went with your earlier suggestion to switch to `for int i...` with a `int place = i + 1` which handled this, I think ___ *[_includes/v19.1/app/BasicExample.java, line 155 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgEAx-z9OVmAiiXN2lA:-LgdINaf0lYWbAe3eBN2:b34ymx8) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L155)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> maybe worth adding a comment saying that we know that `getResultSet` will not return `null` if `pstmt.execute()` was true </blockquote></details> Added this comment. ___ *[_includes/v19.1/app/BasicExample.java, line 156 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgEABq6-RwySx-jKXHc:-LgdIVMi1ldgjjMT-g8N:b-p08yzu) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L156)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> is it worth being consistent about making local variables `final` whenever possible? it doesn't seem like a big deal to me, but i think my natural approach would have just been to not make any of the local variables final </blockquote></details> Cool, I was mostly cargo-culting the `final`. I removed `final` from all of the local vars. It's left on some of the class vars in `BasicExample` but presumably they're fine there? (I am not a real Java programmer, just trying to play one on TV, so let me know :-} ) ___ *[_includes/v19.1/app/BasicExample.java, line 170 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgECRKC5DLiVvewgvk0:-LgdKCOy8G0fyVBStBcj:bqwxnsk) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L170)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> the line lengths of the comment could be longer </blockquote></details> Updated to use ~94 cols for easier reading ___ *[_includes/v19.1/app/BasicExample.java, line 205 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgECBjQ4xD4i816t3Ol:-LgcMcil8F7TbvUmEMry:b-jwktn8) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L205)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> i am not sure if this is necessary. do you know if we need it for sure? </blockquote></details> Based on my quick testing, we do not appear to need it for the retries to be handled. Removed that line. ___ *[_includes/v19.1/app/BasicExample.java, line 244 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgECiEs5M6tLL3DqxKH:-LgdMJZ5AExy_VFYGihn:byl0t24) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L244)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> technically speaking, shouldn't both of these selects and the UPSERT below all be part of the same transaction? if this example isn't concerned with that level of concurrent access it's probably fine to leave, but maybe worth adding a comment explaining that </blockquote></details> Thanks, wasn't thinking about that. Went ahead and pushed the account transfer logic to the database by writing it as a single SQL stmt, and added a CHECK constraint during table creation so the balance must be >= 0. Please take a look. ___ *[_includes/v19.1/app/BasicExample.java, line 260 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgEEVVO3h94McaPWRtH:-LgdNaXL3iUaNYi2AgDQ:b-3ncgit) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L260)):* > if reusing is desired, then runSQL could be modified to return an Integer for the result (and null if it was an update statement or not found). then this method could be implemented by Changed the implementation to return an int. Please take a look, I suspect it could be more idiomatic. For example I'm returning a -1 in addition to signaling the error condition which I don't think is very Java-esque (but I don't know). Unfortunately I'm not sure how to add the null check code, since I was getting compiler errors that the method's return type should be an int - if you think it's important, please let me know how to do it. Alternatively, please feel free to push edits to this PR if that is easier for you than describing stuff to a Java novice :-) > (if you do that, then the printing logic that is currently in runSQL should probably be pulled out into the calling methods) Would it be sufficient to add a comment that the printing logic is just for debugging and not prod? Reasoning for not removing the printing logic entirely is because it's printing out the PreparedStatement's SQL which will not be easily accessible to callers of `runSQL` (right? or is there a way?). Trying to balance showing good code vs. being really explicit for teaching/demonstration purposes In any case here is the comment I added to runSQL's remaining printing logic - let me know if you think it's sufficient "This printed output is for debugging and/or demonstration purposes only. It would not be necessary in production code." > separately -- method implementation details might belong better as a comment inside the method rather than on the method cool - made all of these into javadocs instead ___ *[_includes/v19.1/app/BasicExample.java, line 262 at r1](https://reviewable.io/reviews/cockroachdb/docs/4855#-LgEFJd118XsXgzxo03j:-LgcN2Ip2TkfeDzAqJbu:b-njzfzr) ([raw file](https://github.com/cockroachdb/docs/blob/70ecb331b4798e47a5237766fbf434c9d34447d2/_includes/v19.1/app/BasicExample.java#L262)):* <details><summary><i>Previously, rafiss (Rafi Shamim) wrote…</i></summary><blockquote> sid seems unused </blockquote></details> Removed, thanks! <!-- Sent from Reviewable.io -->
|
Whoa, looks like Reviewable really messed up the Github comment output on this one. Everything looks OK to me on the actual review page tho |
There was a problem hiding this comment.
Reviewable status:
_includes/v19.1/app/BasicExample.java, line 83 at r1 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
JDBI does look nice!
I think the goal here is to use standard Java stuff, but hopefully it's easy for a Java developer to translate into something else if they need (?). Based on the way our docs/etc. are laid out currently, I think if we did add some instructions for JDBI it would be in a separate doc like we have for Hibernate
that sounds good to me!
_includes/v19.1/app/BasicExample.java, line 93 at r1 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
Changed all the method comments to javadoc, and added instructions in comment block at top of file for building the javadoc
awesome, thanks!
_includes/v19.1/app/BasicExample.java, line 148 at r1 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
Updated everywhere to use
} else {(aka "cuddled elses")
the catches could be cuddled too but i'm not going to block anything on that
_includes/v19.1/app/BasicExample.java, line 156 at r1 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
Cool, I was mostly cargo-culting the
final. I removedfinalfrom all of the local vars. It's left on some of the class vars inBasicExamplebut presumably they're fine there? (I am not a real Java programmer, just trying to play one on TV, so let me know :-} )
yeah they're fine to keep around, there's generally no major impact. the compiler can detect anything that's implicitly final, and the only other reason is to make sure someone doesn't reassign a variable when they shouldn't but that's not a big deal
_includes/v19.1/app/BasicExample.java, line 244 at r1 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
Thanks, wasn't thinking about that. Went ahead and pushed the account transfer logic to the database by writing it as a single SQL stmt, and added a CHECK constraint during table creation so the balance must be >= 0. Please take a look.
yeah this seems like it would work. i believe the more idiomatic way would be to use a transaction explicitly, with BEGIN and COMMIT statements but i also think that will make this example too long so it isn't worth doing that. but i do think a comment explaining that BEGIN/COMMIT could be used here may help though.
_includes/v19.1/app/BasicExample.java, line 260 at r1 (raw file):
compiler errors that the method's return type should be an int
it seems like that was happening because the return type is int -- but in java that type is distinct from Integer. int is a primitive type and cannot be null, while Integer is an Object that wraps the primitive type with an immutable reference. for me, the main benefit i saw in making runSQL() return an Integer is that then the implementation of getAccountBalance() would be much simpler and could just call out to runSQL(). but that only works if runSQL() returns the result of a SELECT query, which is different than what it's doing now -- i'll leave a comment on that method
Would it be sufficient to add a comment that the printing logic is just for debugging and not prod?
that seems totally fine
made all of these into javadocs instead
ah, actually what i meant before is that the javadoc generally describes "what the method does" and it goes above the method; but for any comment that describes "how the method works" it should be a normal comment inside the method body. this rule is definitely not well-adhered to in most java code i've seen so don't sweat too much on moving comments around, but just FYI. i'm certainly not particular about where the comments end up in this PR
_includes/v19.1/app/BasicExample.java, line 139 at r2 (raw file):
* @param args String Varargs to fill in the SQL code's * placeholders. * @return Number of rows updated, or -1 if an error is thrown.
i feel like it would be useful to make the return value something like this:
- if there was any error, return -1
- for SELECT:
-- return result of the query
-- ornullif no result was found - for INSERT/UPDATE
-- return number of rows updated
just a suggestion though. if you feel like this is overloading the return value too much, feel free to not do this. if you go ahead with it, a comment explaining that we're only multi-purposing the return value for the sake of this example code might be prudent.
also see my reply below -- to return null the return value should be an Integer instead of an int
_includes/v19.1/app/BasicExample.java, line 221 at r2 (raw file):
} } rv++;
if rv is supposed to be the number of updated rows, then it shouldn't change here, since this branch is only for SELECT statements
_includes/v19.1/app/BasicExample.java, line 317 at r2 (raw file):
String sAmount = Integer.toString(amount); return runSQL("UPSERT INTO accounts (id, balance) VALUES (?, ((SELECT balance FROM accounts WHERE id = ?) - ?)), (?, ((SELECT balance FROM accounts WHERE id = ?) + ?))",
consider breaking up the string into two lines and concatenate
rmloveland
force-pushed
the
20190528-simplify-jdbc-example
branch
from
e3fcc9e to
b5dd9ce
Compare
There was a problem hiding this comment.
Reviewable status:
_includes/v19.1/app/BasicExample.java, line 148 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
the catches could be cuddled too but i'm not going to block anything on that
Cuddled 'em just cause -- it's easy so why not
_includes/v19.1/app/BasicExample.java, line 244 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
yeah this seems like it would work. i believe the more idiomatic way would be to use a transaction explicitly, with
BEGINandCOMMITstatements but i also think that will make this example too long so it isn't worth doing that. but i do think a comment explaining that BEGIN/COMMIT could be used here may help though.
Added a comment that BEGIN/COMMIT were omitted for brevity -- also linked to CRDB docs showing that individual statements are treated as implicit txns
_includes/v19.1/app/BasicExample.java, line 260 at r1 (raw file):
the main benefit i saw in making runSQL() return an Integer is that then the implementation of getAccountBalance() would be much simpler and could just call out to runSQL(). but that only works if runSQL() returns the result of a SELECT query, which is different than what it's doing now
Responded to this in the other comment on runSQL
_includes/v19.1/app/BasicExample.java, line 139 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i feel like it would be useful to make the return value something like this:
- if there was any error, return -1
- for SELECT:
-- return result of the query
-- ornullif no result was found- for INSERT/UPDATE
-- return number of rows updatedjust a suggestion though. if you feel like this is overloading the return value too much, feel free to not do this. if you go ahead with it, a comment explaining that we're only multi-purposing the return value for the sake of this example code might be prudent.
also see my reply below -- to return
nullthe return value should be anIntegerinstead of anint
I updated to Integer and removed the useless increment of rv++ in the SELECT case as mentioned in another comment.
However I'm not clear on how to handle the SELECT as you describe given my current Java skills and time available to work on this. Do you think the code with the updates described above minus the SELECT suggestion can work for this example? I hope it will at least be clear to folks who know Java well what to do. What do you think?
_includes/v19.1/app/BasicExample.java, line 221 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
if
rvis supposed to be the number of updated rows, then it shouldn't change here, since this branch is only for SELECT statements
Fixed by removing (also removed erroneous use of this for a SELECT call in main).
_includes/v19.1/app/BasicExample.java, line 317 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
consider breaking up the string into two lines and concatenate
Done.
There was a problem hiding this comment.
thanks for all the changes! lgtm
Reviewable status:
_includes/v19.1/app/BasicExample.java, line 139 at r2 (raw file):
Previously, rmloveland (Rich Loveland) wrote…
I updated to
Integerand removed the useless increment ofrv++in the SELECT case as mentioned in another comment.However I'm not clear on how to handle the SELECT as you describe given my current Java skills and time available to work on this. Do you think the code with the updates described above minus the
SELECTsuggestion can work for this example? I hope it will at least be clear to folks who know Java well what to do. What do you think?
yup, seems fine. this seems clear enough to me
|
Thank you @rafiss !!!!!!1! Apologies in advance for a bit of forthcoming noise, I have to make some uninteresting updates re: secure connections / certs now that we have nailed down the main code samples -- then it goes in for docs team internal review. |
rmloveland
force-pushed
the
20190528-simplify-jdbc-example
branch
from
b5dd9ce to
a739028
Compare
There was a problem hiding this comment.
Reviewable status:
v19.1/build-a-java-app-with-cockroachdb.md, line 13 at r3 (raw file):
</div> This tutorial shows you how build a simple Java application with CockroachDB using a PostgreSQL-compatible driver or ORM.
nit: "how build" > "how to build"
v19.1/build-a-java-app-with-cockroachdb.md, line 63 at r3 (raw file):
| 4. Insert random account data using JDBC's bulk insertion support | `BasicExampleDAO.bulkInsertRandomAccountData()` | | 5. Print out some account data | `BasicExampleDAO.readAccounts(int limit)` | | 6. Drop the `accounts` table and performs any other necessary cleanup | `BasicExampleDAO.tearDown()` (This cleanup step means you can run this program more than once.) |
nit: "performs" > "perform"
v19.1/build-a-java-app-with-cockroachdb.md, line 70 at r3 (raw file):
1. Download [`BasicExample.java`](https://raw.githubusercontent.com/cockroachdb/docs/master/_includes/v19.1/app/BasicExample.java), or create the file yourself and copy the code below. 2. Download [the PostgreSQL JDBC driver](https://jdbc.postgresql.org/download.html).
We already did this, right? 🤔
v19.1/build-a-java-app-with-cockroachdb.md, line 75 at r3 (raw file):
{% include copy-clipboard.html %} ~~~ shell $ javac -classpath .:/path/to/postgresql.jar BasicExample.java
I had to use needed .: before thefor the execute command but it gave me error for the compile command. Don't know why though
v19.1/build-a-java-app-with-cockroachdb.md, line 228 at r3 (raw file):
We strongly recommend setting `rewriteBatchedInserts=true`; we have seen 2-3x performance improvements with it enabled. From [the JDBC connection parameters documentation](https://jdbc.postgresql.org/documentation/head/connect.html#connection-parameters): > This will change batch inserts from `insert into foo (col1, col2, col3) values (1,2,3)` into `insert into foo (col1, col2, col3) values (1,2,3), (4,5,6)` this provides 2-3x performance improvement
nit: "this provides" > "provides"
rmloveland
force-pushed
the
20190528-simplify-jdbc-example
branch
from
a739028 to
13c50fa
Compare
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/13c50fa650ef2f963333d6fcfded6bfec8d7fc65/ Edited pages: |
There was a problem hiding this comment.
Reviewable status:
_includes/v19.1/app/BasicExample.java, line 50 at r4 (raw file):
BasicExampleDAO dao = new BasicExampleDAO(ds); // Test our retry handling logic, maybe (if FORCE_RETRY is
What is the maybe about? Feels vague.
_includes/v19.1/app/BasicExample.java, line 65 at r4 (raw file):
System.out.printf("BasicExampleDAO.updateAccounts:\n => %s total updated accounts\n", updatedAccounts); // How much money is in account ID 1?
Doesn't this get and print the balances of both accounts, not just 1?
Summary of changes: - Update 'Build a Java App with CockroachDB' to use: - the Java DAO pattern - JDBC - code for automatically retrying in case of txn retry errors - an example of fast bulk insertion using JDBC batching - Add a 'Recommended Practices' section that includes information about IMPORT, recommended batch size, and the JDBC INSERT rewriting flag Does all of the above for versions 19.1 and 19.2. Fixes #4621, #3578, #4399.
rmloveland
force-pushed
the
20190528-simplify-jdbc-example
branch
from
13c50fa to
e69a0d0
Compare
There was a problem hiding this comment.
Thanks so much for the thorough reviews Rafi, Amruta, and Jesse!
Reviewable status:
_includes/v19.1/app/BasicExample.java, line 50 at r4 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
What is the
maybeabout? Feels vague.
Fixed in 19.1 and 19.2 examples (secure & insecure)
_includes/v19.1/app/BasicExample.java, line 65 at r4 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Doesn't this get and print the balances of both accounts, not just 1?
Yes. Fixed in 19.1 and 19.2 examples (secure & insecure)
v19.1/build-a-java-app-with-cockroachdb.md, line 13 at r3 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
nit: "how build" > "how to build"
Fixed (here and in 19.2)
v19.1/build-a-java-app-with-cockroachdb.md, line 63 at r3 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
nit: "performs" > "perform"
Fixed.
v19.1/build-a-java-app-with-cockroachdb.md, line 70 at r3 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
We already did this, right? 🤔
Fixed. Nice catch!
v19.1/build-a-java-app-with-cockroachdb.md, line 75 at r3 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
I had to use needed
.:before thefor the execute command but it gave me error for the compile command. Don't know why though
Hm, I'm not sure. Can you share the error message?
v19.1/build-a-java-app-with-cockroachdb.md, line 228 at r3 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
nit: "this provides" > "provides"
I would fix this but it's copied verbatim from the PG docs, so I'm hesitant to rewrite a quotation
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/e69a0d06937c0d8623ee4da0d07e2930c76fefb6/ Edited pages: |
Fixes #4621, #3578, #4399.
Summary of changes:
Update 'Build a Java App with CockroachDB' to use the Java DAO pattern
Add a 'Recommended Practices' section that includes information about
IMPORT, recommended batch size, and the JDBC INSERT rewriting flag
TODO:
this code is reviewed by someone with Java expertise and updated to
match good style, etc. Once that is done, the secure code sample in
this PR will be updated with the necessary cert munging.