Skip to content
This repository has been archived by the owner on Nov 14, 2024. It is now read-only.

support persistables in v2 tables #2495

Merged
merged 11 commits into from
Oct 27, 2017
Merged

support persistables in v2 tables #2495

merged 11 commits into from
Oct 27, 2017

Conversation

nziebart
Copy link
Contributor

@nziebart nziebart commented Oct 16, 2017 •
edited
Loading

Goals (and why):
Support persistables in v2 generated code

Fixes #2485

Implementation Description (bullets):

  • Expose a getJavaObjectTypeClass method on ColumnValue, which correctly returns the underlying class
  • Add a column to the test schema containing a persistable

TODO:

  • add docs indicating this feature is beta

Concerns (what feedback would you like?):

Where should we start reviewing?:

Priority (whenever / two weeks / yesterday):
this week

This change is Reviewable

@nziebart nziebart changed the title support persistables support persistables in v2 tables Oct 16, 2017
@nziebart nziebart force-pushed the nziebart/support-persistable branch from 7aff01e to 55ac590 Compare October 16, 2017 18:20
@nziebart nziebart requested a review from hsaraogi October 16, 2017 18:32
@nziebart nziebart force-pushed the nziebart/support-persistable branch 2 times, most recently from 6ac5f84 to ec304b6 Compare October 16, 2017 18:35
@nziebart
Copy link
Contributor Author

nziebart commented Oct 16, 2017 •
edited
Loading

TODO: we also want a test for multiple row components, but will do this in a follow up PR

nziebart
nziebart commented Oct 16, 2017
View reviewed changes
...tegrationInput/java/com/palantir/atlasdb/table/description/generated/SchemaApiTestTable.java Outdated
put(ImmutableMultimap.of(row, Column2.of(value)));
}

public void putColumn2(Map<SchemaApiTestRow, String> map) {
public void putColumn2(Map<SchemaApiTestRow, com.palantir.atlasdb.table.description.StringValue> map) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, need to get rid of these package qualifiers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i don't think this behavior changed for the v1 generated code, so leaving as is

@nziebart nziebart force-pushed the nziebart/support-persistable branch from ec304b6 to 98b9018 Compare October 17, 2017 11:06
@codecov-io
Copy link

codecov-io commented Oct 17, 2017 •
edited
Loading

Codecov Report

Merging #2495 into develop will increase coverage by 0.07%.
The diff coverage is 70.68%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2495      +/-   ##
=============================================
+ Coverage      60.11%   60.18%   +0.07%     
- Complexity      2474     4080    +1606     
=============================================
  Files            855      855              
  Lines          40065    40067       +2     
  Branches        4044     4045       +1     
=============================================
+ Hits           24084    24115      +31     
+ Misses         14507    14471      -36     
- Partials        1474     1481       +7
Impacted Files Coverage Δ Complexity Δ
...tir/atlasdb/table/description/TableDefinition.java 70.43% <ø> (ø) 37 <0> (+37) ⬆️
...ir/atlasdb/table/description/render/Renderers.java 81.81% <100%> (+0.42%) 12 <0> (+12) ⬆️
...asdb/table/description/ColumnValueDescription.java 44.24% <45.45%> (+5.35%) 23 <0> (+23) ⬆️
.../palantir/atlasdb/table/description/ValueType.java 47.14% <66.66%> (+1.93%) 6 <3> (+6) ⬆️
...table/description/render/TableClassRendererV2.java 80% <80.76%> (ø) 0 <0> (ø) ⬇️
...ain/java/com/palantir/paxos/PaxosAcceptorImpl.java 73.21% <0%> (ø) 0% <0%> (ø) ⬇️
...alantir/lock/client/LockRefreshingLockService.java 60.29% <0%> (+1.47%) 11% <0%> (+11%) ⬆️
...ain/java/com/palantir/paxos/PaxosProposerImpl.java 88.33% <0%> (+3.33%) 0% <0%> (ø) ⬇️
...e/description/render/NamedColumnValueRenderer.java 92.92% <0%> (+3.53%) 17% <0%> (+17%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 820930b...c2e3816. Read the comment docs.

hsaraogi
hsaraogi reviewed Oct 17, 2017
View reviewed changes
...grationInput/java/com/palantir/atlasdb/table/description/generated/SchemaApiTestV2Table.java
@@ -166,7 +167,7 @@ public Namespace getNamespace() {

/**
* Returns the value for column Column2 and specified row components. */
public Optional<String> getColumn2(String component1) {
public Optional<StringValue> getColumn2(String component1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not compile for me:

/Users/hsaraogi/code/atlasdb/atlasdb-client/src/integrationInput/java/com/palantir/atlasdb/table/description/generated/SchemaApiTestV2Table.java:330: error: cannot find symbol
            StringValue newValue = processor.apply(result.get());
            ^
  symbol:   class StringValue
  location: class SchemaApiTestV2Table
33 errors
:atlasdb-client:compileIntegrationInputJava FAILED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, not compiling in circle either. let me fix it

@nziebart nziebart assigned nziebart and unassigned hsaraogi Oct 18, 2017
@gsheasby gsheasby mentioned this pull request Oct 19, 2017
Closed
@nziebart nziebart force-pushed the nziebart/support-persistable branch from a69c2fd to 9033ef8 Compare October 20, 2017 11:56
@fsamuel-bs fsamuel-bs assigned fsamuel-bs and hsaraogi and unassigned nziebart and fsamuel-bs Oct 24, 2017
@fsamuel-bs
Copy link
Contributor

This should be good for review again. Pinging @hsaraogi for it.

@hsaraogi
Copy link
Contributor

Reviewed 3 of 14 files at r1, 2 of 2 files at r2, 10 of 12 files at r4, 2 of 3 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

hsaraogi
hsaraogi reviewed Oct 26, 2017
View reviewed changes
Copy link
Contributor

@hsaraogi hsaraogi left a comment

Choose a reason for hiding this comment

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

LGTM overall, one comment on release notes.

docs/source/release_notes/release-notes.rst Outdated
- Simplify and annotate the constructors for ``SerializableTransactionManager``. This should make the code of that class more maintainable.
If you used one of the deleted or deprecated constructors, use the static ``create`` method.
(`Pull Request <https://github.com/palantir/atlasdb/pull/2549>`__)
* - |improved| |devbreak|
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the merge conflict wasnt resolved in the right manner, this PR is effectively adding two release note entries.

@fsamuel-bs fsamuel-bs force-pushed the nziebart/support-persistable branch from 5fec045 to 8f80cfc Compare October 26, 2017 15:14
@hsaraogi hsaraogi merged commit 11fff09 into develop Oct 27, 2017
@hsaraogi hsaraogi deleted the nziebart/support-persistable branch October 27, 2017 11:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hsaraogi hsaraogi hsaraogi approved these changes

Assignees

@hsaraogi hsaraogi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nziebart @codecov-io @fsamuel-bs @hsaraogi