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

Remove the conjure-java-jersey-server dependency #6213

Merged
merged 4 commits into from
Sep 9, 2022
+9 −7
Merged

Remove the conjure-java-jersey-server dependency #6213

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
968f495
Remove the conjure-java-jersey-server dependency
mglazer Sep 1, 2022
efa971f
Fix
mglazer Sep 1, 2022
73e8f3b
Use LoggingArgs.safeInternalTableName(tableName)
schlosna Sep 9, 2022
4baefc4
Merge remote-tracking branch 'origin/develop' into feature/remove-con…
schlosna Sep 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion atlasdb-impl-shared/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ dependencies {

implementation 'com.github.ben-manes.caffeine:caffeine'
implementation 'com.palantir.common:streams'
implementation 'com.palantir.conjure.java.runtime:conjure-java-jersey-server'
implementation 'com.palantir.conjure.java.runtime:conjure-java-jackson-serialization'
implementation 'com.palantir.safe-logging:safe-logging'
implementation 'com.palantir.safe-logging:preconditions'
Expand Down Expand Up @@ -70,6 +69,7 @@ dependencies {
testImplementation 'com.palantir.conjure.java.api:errors'
testImplementation 'com.palantir.conjure.java.api:service-config'
testImplementation 'com.palantir.conjure.java.api:ssl-config'
testImplementation 'com.palantir.conjure.java.runtime:conjure-java-jersey-server'
testImplementation 'com.palantir.conjure.java.runtime:conjure-java-jaxrs-client'
testImplementation 'com.palantir.refreshable:refreshable'
testImplementation 'io.dropwizard.metrics:metrics-core'
Expand Down
14 changes: 8 additions & 6 deletions atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/sweep/SweeperServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import com.palantir.atlasdb.keyvalue.api.SweepResults;
import com.palantir.atlasdb.keyvalue.api.TableReference;
import com.palantir.atlasdb.logging.LoggingArgs;
import com.palantir.conjure.java.server.jersey.WebPreconditions;
import com.palantir.logsafe.Preconditions;
import com.palantir.logsafe.SafeArg;
import com.palantir.logsafe.UnsafeArg;
import com.palantir.logsafe.logger.SafeLogger;
Expand Down Expand Up @@ -126,16 +126,18 @@ private SweepBatchConfig buildConfigWithOverrides(
}

private TableReference getTableRef(String tableName) {
WebPreconditions.checkArgument(
TableReference.isFullyQualifiedName(tableName), "Table name {} is not fully qualified", tableName);
Preconditions.checkArgument(
TableReference.isFullyQualifiedName(tableName),
"Table name is not fully qualified",
UnsafeArg.of("tableName", tableName));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can use LoggingArgs.safeInternalTableName here.

Copy link
Contributor

Choose a reason for hiding this comment

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

updated both

return TableReference.createFromFullyQualifiedName(tableName);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we should clean up TableReference to use the com.palantir.logsafe.Preconditions instead of Guava, then we could get rid of this getTableRef(String) method and just inline the TableReference.createFromFullyQualifiedName(tableName)

    public static TableReference createFromFullyQualifiedName(String fullTableName) {
        int index = fullTableName.indexOf('.');
        Preconditions.checkArgument(
                index > 0, "Table name is not a fully qualified table name.", UnsafeArg.of("tableName", fullTableName));
        return create(
                Namespace.create(fullTableName.substring(0, index), Namespace.UNCHECKED_NAME),
                fullTableName.substring(index + 1));
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have errorprones which will automatically perform that migration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Negative Ghostrider. I ran ./gradlew compileJava compileTestJava -PerrorProneApply=PreferSafeLoggingPreconditions against develop but that didn't generate any changes as I imagine since the aren't Args we're hitting the path without suggested fix. Separately we could make that smarter and generate UnsafeArgs, but don't think we should block on this.

https://github.com/palantir/gradle-baseline/blob/828111050da72901a66a676ae517719a9e9d9849/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLoggingPreconditions.java#L88-L120

Copy link
Contributor

Choose a reason for hiding this comment

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

Draft PR #6223 to migrate more Guava -> logsafe Preconditions

}

private void checkTableExists(String tableName, TableReference tableRef) {
WebPreconditions.checkArgument(
Preconditions.checkArgument(
specificTableSweeper.getKvs().getAllTableNames().contains(tableRef),
"Table requested to sweep %s does not exist",
tableName);
"Table requested to sweep does not exist",
UnsafeArg.of("tableName", tableName));
Copy link
Contributor

Choose a reason for hiding this comment

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

LoggingArgs.safeInternalTableName

}

private SweepResults runFullSweepWithoutSavingResults(
Expand Down