You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/05/25 20:52:13 UTC
[GitHub] [bookkeeper] pkumar-singh opened a new pull request #2723: Refactor book keeper cluster test case
pkumar-singh opened a new pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723
### Motivation
BookKeeperClusterTestCase has historically exposed its members to all
subclasses, which would then manipulate them in many ways. There was
an array of objects for configurations, bookieServers, autorecovery,
which implicit linking between the objects based on maps and indices.
Individual subclasses manipulated these arrays.
This makes it hard to add any dependency injection on the objects
managed by BookKeeperClusterTestCase as the objects. To add DI, we
need each object to have a bunch of other objects associated with
it. For example, for each Bookie, we need to create the
Journal. Maintaining these in separate arrays will lead to fragile
tests.
This change encapsulates all the testing objects in a per bookie
object, and only allows manipulation through methods. This will allow
us to group the objects needed for DI clearly.
Disable testFollowBookieAddressChangeTrckingDisabled
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on a change in pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
hsaputra commented on a change in pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723#discussion_r639971234
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
##########
@@ -504,6 +504,12 @@ public boolean isSecure() {
};
}
+ public static String buildStatsLoggerScopeName(BookieId addr) {
+ StringBuilder nameBuilder = new StringBuilder();
+ nameBuilder.append(addr.toString().replace('.', '_').replace('-', '_').replace(":", "_"));
Review comment:
Probably do not need StringBuilder for this.
Just need to return `addr.toString().replace('.', '_').replace('-', '_').replace(":", "_")`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra commented on pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
hsaputra commented on pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723#issuecomment-849865748
Thanks for the reviews - will merge this PR by EOD PST if no more comments or concerns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723#discussion_r640025675
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
##########
@@ -504,6 +504,12 @@ public boolean isSecure() {
};
}
+ public static String buildStatsLoggerScopeName(BookieId addr) {
Review comment:
Addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a change in pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723#discussion_r639983590
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
##########
@@ -504,6 +504,12 @@ public boolean isSecure() {
};
}
+ public static String buildStatsLoggerScopeName(BookieId addr) {
Review comment:
The name of this method can be misleading. We already removed the bookie address from the metric names (instead it's added as a label). Sine this appears to be only by tests, we should just move the methods to tests folder.
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieWriteToJournalTest.java
##########
@@ -80,7 +79,9 @@ void readJournal() throws IOException, BookieException {
/**
* test that Bookie calls correctly Journal.logAddEntry about "ackBeforeSync" parameter.
*/
+
@Test
+// @Ignore("PLSR-1850 test is failing due to Bookie interface change")
Review comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on a change in pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on a change in pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723#discussion_r640026060
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
##########
@@ -504,6 +504,12 @@ public boolean isSecure() {
};
}
+ public static String buildStatsLoggerScopeName(BookieId addr) {
+ StringBuilder nameBuilder = new StringBuilder();
+ nameBuilder.append(addr.toString().replace('.', '_').replace('-', '_').replace(":", "_"));
Review comment:
Yes. Addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] pkumar-singh commented on pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
pkumar-singh commented on pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723#issuecomment-849833205
@merlimat Addressed your comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hsaputra merged pull request #2723: Refactor book keeper cluster test case
Posted by GitBox <gi...@apache.org>.
hsaputra merged pull request #2723:
URL: https://github.com/apache/bookkeeper/pull/2723
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org