-
Notifications
You must be signed in to change notification settings - Fork 116
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 option for table prefix to DynamoDB #7598
Changes from 8 commits
cb8fbe8
9be25e4
3c7d8d4
2b727bc
2967f08
99267f2
4dd0f46
fa4cc50
7938496
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
/* | ||
* Copyright (C) 2023 Dremio | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.projectnessie.quarkus.config; | ||
|
||
import io.quarkus.runtime.annotations.StaticInitSafe; | ||
import io.smallrye.config.ConfigMapping; | ||
import java.util.Optional; | ||
|
||
@StaticInitSafe | ||
@ConfigMapping(prefix = "nessie.version.store.persist.dynamodb") | ||
public interface QuarkusDynamoDBConfig { | ||
Optional<String> tablePrefix(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,9 +40,21 @@ public String getName() { | |
return DynamoDBBackendFactory.NAME; | ||
} | ||
|
||
@SuppressWarnings("ClassEscapesDefinedScope") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IntelliJ flags this annotation as redundant... Could you double check, please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've removed it. Although interestingly my IntilliJ complains when I remove the annotation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does it say? My EAP 2 build is happy :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it might be complaining about returning the package-private There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correction: I was running my old IJ built for this review before and it did not give that warning. I switched to EAP 3 (just now) and it does indeed issue the warning. Having considered the code carefully, I think the warning is justified. I also see some usages that rely on the more specific return type... so cleaning this up is probably beyond the scope of this PR. Let's not suppress the warning, though, as a reminder to revisit this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have the same issue in BigTable impl... we return |
||
@Override | ||
public DynamoDBBackend createNewBackend() { | ||
return new DynamoDBBackend(buildNewClient(), true); | ||
return createNewBackend(dynamoDBConfigBuilder().build(), true); | ||
} | ||
|
||
@SuppressWarnings("ClassEscapesDefinedScope") | ||
public DynamoDBBackend createNewBackend( | ||
DynamoDBBackendConfig dynamoDBBackendConfig, boolean closeClient) { | ||
return new DynamoDBBackend(dynamoDBBackendConfig, closeClient); | ||
} | ||
|
||
@VisibleForTesting | ||
public ImmutableDynamoDBBackendConfig.Builder dynamoDBConfigBuilder() { | ||
return DynamoDBBackendConfig.builder().client(buildNewClient()); | ||
} | ||
|
||
@VisibleForTesting | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially, although I did not modify the body of that test at all so the duplication was there already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK let's leave it, as it's not related to your changes 👍