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/09/16 09:38:19 UTC
[GitHub] [bookkeeper] RaulGracia opened a new pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
RaulGracia opened a new pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796
### Motivation
If a Bookie uses the new Bookie ID scheme explicitly (set via configuration), it should prevail over default network information to identify the Bookie and lookup for cookies in Zookeeper. Otherwise, the Bookie may fetch legacy cookies related to the same hostname but are invalid because the id differs. Essentially, this PR tries to choose between a configuration-based Bookie ID or a default network info ID when looking for cookies in Zookeeper (considering both is problematic in some cases).
### Changes
The change is simple: when collecting the Bookie IDs that will be used to fetch cookies from Zookeeper (`BookieImpl.possibleBookieIds()`), the code now checks if there is a Bookie ID passed by configuration. If so, we just used that one for lookup cookies in Zookeeper. Otherwise, we keep the existing behavior of using network information to fetch cookies from Zookeeper.
Master Issue: #2795
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a change in pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#discussion_r722966244
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
##########
@@ -1614,4 +1614,34 @@ private void testInvalidServiceMetadataURICase(String uri) throws Exception {
// ok
}
}
+
+ @Test
+ public void testBookieIdSetting() throws Exception {
+ String customBookieId = "customId";
+ // If BookieID is set, it takes precedence over network info.
+ final ServerConfiguration conf = newServerConfiguration().setBookieId(customBookieId);
+ BookieServer server = new MockBookieServer(conf);
+ server.start();
+ assertEquals(customBookieId, server.getBookieId().toString());
+ server.shutdown();
+ }
+
+ @Test(expected = BookieException.InvalidCookieException.class)
+ public void testBookieIdChange() throws Exception {
+ // By default, network info is set as Bookie Id and it is stored in the Cookie.
+ final ServerConfiguration conf = newServerConfiguration();
+ BookieServer server = new MockBookieServer(conf);
+ server.start();
+ assertNotNull(server.getBookieId().toString());
+ server.shutdown();
+
+ // If BookieID is set, it takes precedence over network info. Because of that, the new Bookie start
+ // should fail with an InvalidCookieException, as now the custom BookieID takes precedence.
+ String customBookieId = "customId";
+ conf.setBookieId(customBookieId);
+ server = new MockBookieServer(conf);
+ server.start();
+ assertEquals(customBookieId, server.getBookieId().toString());
+ server.shutdown();
Review comment:
very good point @dlg99
we should verify the Exception in a `catch` block and call "shutdown" in a `finally` block in order to perform any cleanup that is needed (and do not fall into leaks)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#issuecomment-920887677
This change makes sense to me.
The upgrade story for enabling BookieID is to compute the same BookieID as it is written in the bookie, like you did in Pravega, is this correct ?
We should write this somewhere in the docs (not in the scope of this PR)
In order to finalise the patch we have to add a unit test, probably a unit test about this method that you changed is enough.
A test that shows how to switch to BookieID will be also very good
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] RaulGracia commented on a change in pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
RaulGracia commented on a change in pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#discussion_r723031628
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
##########
@@ -1614,4 +1614,34 @@ private void testInvalidServiceMetadataURICase(String uri) throws Exception {
// ok
}
}
+
+ @Test
+ public void testBookieIdSetting() throws Exception {
+ String customBookieId = "customId";
+ // If BookieID is set, it takes precedence over network info.
+ final ServerConfiguration conf = newServerConfiguration().setBookieId(customBookieId);
+ BookieServer server = new MockBookieServer(conf);
+ server.start();
+ assertEquals(customBookieId, server.getBookieId().toString());
+ server.shutdown();
+ }
+
+ @Test(expected = BookieException.InvalidCookieException.class)
+ public void testBookieIdChange() throws Exception {
+ // By default, network info is set as Bookie Id and it is stored in the Cookie.
+ final ServerConfiguration conf = newServerConfiguration();
+ BookieServer server = new MockBookieServer(conf);
+ server.start();
+ assertNotNull(server.getBookieId().toString());
+ server.shutdown();
+
+ // If BookieID is set, it takes precedence over network info. Because of that, the new Bookie start
+ // should fail with an InvalidCookieException, as now the custom BookieID takes precedence.
+ String customBookieId = "customId";
+ conf.setBookieId(customBookieId);
+ server = new MockBookieServer(conf);
+ server.start();
+ assertEquals(customBookieId, server.getBookieId().toString());
+ server.shutdown();
Review comment:
Thanks for the feedback. Now the test catches the expected exception and shuts down the mock server at the end of the test.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli merged pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
eolivelli merged pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] RaulGracia commented on pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
RaulGracia commented on pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#issuecomment-930055876
@eolivelli added a couple of tests to check the behavior of using Bookie ID in the configuration.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
dlg99 commented on a change in pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#discussion_r722496673
##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
##########
@@ -1614,4 +1614,34 @@ private void testInvalidServiceMetadataURICase(String uri) throws Exception {
// ok
}
}
+
+ @Test
+ public void testBookieIdSetting() throws Exception {
+ String customBookieId = "customId";
+ // If BookieID is set, it takes precedence over network info.
+ final ServerConfiguration conf = newServerConfiguration().setBookieId(customBookieId);
+ BookieServer server = new MockBookieServer(conf);
+ server.start();
+ assertEquals(customBookieId, server.getBookieId().toString());
+ server.shutdown();
+ }
+
+ @Test(expected = BookieException.InvalidCookieException.class)
+ public void testBookieIdChange() throws Exception {
+ // By default, network info is set as Bookie Id and it is stored in the Cookie.
+ final ServerConfiguration conf = newServerConfiguration();
+ BookieServer server = new MockBookieServer(conf);
+ server.start();
+ assertNotNull(server.getBookieId().toString());
+ server.shutdown();
+
+ // If BookieID is set, it takes precedence over network info. Because of that, the new Bookie start
+ // should fail with an InvalidCookieException, as now the custom BookieID takes precedence.
+ String customBookieId = "customId";
+ conf.setBookieId(customBookieId);
+ server = new MockBookieServer(conf);
+ server.start();
+ assertEquals(customBookieId, server.getBookieId().toString());
+ server.shutdown();
Review comment:
The test expected exception, comment says to expect exception after the start.
Do you need assert and shutdown after the code that's supposed to throw?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#issuecomment-950460158
Because it's based on an interface change(#2717 ), remove it from release 4.14.3
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] RaulGracia commented on pull request #2796: Issue 2795: Bookkeeper upgrade using Bookie ID may fail due to cookie mismatch
Posted by GitBox <gi...@apache.org>.
RaulGracia commented on pull request #2796:
URL: https://github.com/apache/bookkeeper/pull/2796#issuecomment-930055876
@eolivelli added a couple of tests to check the behavior of using Bookie ID in the configuration.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org