Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.2.0
    • Component/s: None
    • Labels:
      None

      Description

      somewhere, probably zookeeper, we need to keep track of the the information about keys used for access and for mac validation as well as the digest type for entries. we can't write a general recovery tool without it.

      1. BOOKKEEPER-2.diff
        45 kB
        Ivan Kelly
      2. BOOKKEEPER-2.diff
        34 kB
        Ivan Kelly
      3. BOOKKEEPER-2.diff
        33 kB
        Ivan Kelly

        Issue Links

          Activity

          Hide
          Uma Maheswara Rao G added a comment -

          First issue committed in AutoReplication work
          Great work, Ivan and Sijie.

          Show
          Uma Maheswara Rao G added a comment - First issue committed in AutoReplication work Great work, Ivan and Sijie.
          Hide
          Hudson added a comment -

          Integrated in bookkeeper-trunk #611 (See https://builds.apache.org/job/bookkeeper-trunk/611/)
          BOOKKEEPER-2: bookkeeper does not put enough meta-data in to do recovery properly (ivank via sijie) (Revision 1362974)

          Result = ABORTED
          sijie :
          Files :

          • /zookeeper/bookkeeper/trunk/CHANGES.txt
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DigestManager.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/DataFormats.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/proto/DataFormats.proto
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java
          • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
          Show
          Hudson added a comment - Integrated in bookkeeper-trunk #611 (See https://builds.apache.org/job/bookkeeper-trunk/611/ ) BOOKKEEPER-2 : bookkeeper does not put enough meta-data in to do recovery properly (ivank via sijie) (Revision 1362974) Result = ABORTED sijie : Files : /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DigestManager.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerOpenOp.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/DataFormats.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/proto/DataFormats.proto /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookKeeperTest.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java /zookeeper/bookkeeper/trunk/bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java
          Hide
          Sijie Guo added a comment -

          committed as r1362974. thanks Ivan's great work. thanks Uma for reviewing.

          Show
          Sijie Guo added a comment - committed as r1362974. thanks Ivan's great work. thanks Uma for reviewing.
          Hide
          Uma Maheswara Rao G added a comment -

          +1 on the patch, It looks great to me

          Please take care of below tiny nit on committing the patch

          typo: 'adminisative'
          /* For an adminisative open, the default passwords

          Show
          Uma Maheswara Rao G added a comment - +1 on the patch, It looks great to me Please take care of below tiny nit on committing the patch typo: 'adminisative' /* For an adminisative open, the default passwords
          Hide
          Uma Maheswara Rao G added a comment -

          Thanks a lot, Ivan for the update on patch.
          @Sijie, I will take a look on this patch tomorrow morning.

          Show
          Uma Maheswara Rao G added a comment - Thanks a lot, Ivan for the update on patch. @Sijie, I will take a look on this patch tomorrow morning.
          Hide
          Sijie Guo added a comment -

          @Uma, could you take a look at Ivan's new patch?

          Show
          Sijie Guo added a comment - @Uma, could you take a look at Ivan's new patch?
          Hide
          Sijie Guo added a comment -

          the new patch looks good to me. +1.

          Show
          Sijie Guo added a comment - the new patch looks good to me. +1.
          Hide
          Ivan Kelly added a comment -

          New patch addresses comments and fixes some other minor issues.

          1) since we stored passwd and digest type on ledger metadata, if user provided wrong digest type or wrong passwd to open the ledger, why we don't fail the open request?

          If the user provides the wrong password and digest to openLedger, it now fails. However, if the user configures a incorrect recovery password or digest, and the ledger has the correct password in the metadata already, no failure occurs during recovery, as all the necessary information is available.

          2) ensurePasswordUsedForOldLedgers is a kind of backward compatibility test. why not move it to TestBackwardCompat? so we could know all incompatibles from a central entry.

          It needs some things like #verifyFullyReplicated, which are only in BookieRecoveryTest.

          Show
          Ivan Kelly added a comment - New patch addresses comments and fixes some other minor issues. 1) since we stored passwd and digest type on ledger metadata, if user provided wrong digest type or wrong passwd to open the ledger, why we don't fail the open request? If the user provides the wrong password and digest to openLedger, it now fails. However, if the user configures a incorrect recovery password or digest, and the ledger has the correct password in the metadata already, no failure occurs during recovery, as all the necessary information is available. 2) ensurePasswordUsedForOldLedgers is a kind of backward compatibility test. why not move it to TestBackwardCompat? so we could know all incompatibles from a central entry. It needs some things like #verifyFullyReplicated, which are only in BookieRecoveryTest.
          Hide
          Sijie Guo added a comment -

          the patch is good, but I had several questions:

          1) since we stored passwd and digest type on ledger metadata, if user provided wrong digest type or wrong passwd to open the ledger, why we don't fail the open request?

          And also in test cases I found you used fixed digestType to open the ledger. but the test cases would be ran with different digest types. The test case would create a ledger using DigestType.CRC32 and open it using DigestType.MAC. Do we allow doing this?

          +        LedgerHandle lh = bkc.createLedger(3, 2, digestType, passwdA);
          
          +        lh = bkc.openLedgerNoRecovery(ledgerId, BookKeeper.DigestType.MAC, passwdA);
          

          2) ensurePasswordUsedForOldLedgers is a kind of backward compatibility test. why not move it to TestBackwardCompat? so we could know all incompatibles from a central entry.

          Show
          Sijie Guo added a comment - the patch is good, but I had several questions: 1) since we stored passwd and digest type on ledger metadata, if user provided wrong digest type or wrong passwd to open the ledger, why we don't fail the open request? And also in test cases I found you used fixed digestType to open the ledger. but the test cases would be ran with different digest types. The test case would create a ledger using DigestType.CRC32 and open it using DigestType.MAC. Do we allow doing this? + LedgerHandle lh = bkc.createLedger(3, 2, digestType, passwdA); + lh = bkc.openLedgerNoRecovery(ledgerId, BookKeeper.DigestType.MAC, passwdA); 2) ensurePasswordUsedForOldLedgers is a kind of backward compatibility test. why not move it to TestBackwardCompat? so we could know all incompatibles from a central entry.
          Hide
          Sijie Guo added a comment -

          @Uma, sure. I would take a look today.

          Show
          Sijie Guo added a comment - @Uma, sure. I would take a look today.
          Hide
          Uma Maheswara Rao G added a comment -

          nit: BookKeeperAdmin: one unused import for DigestType.
          as we are touching the file, I would suggest clean the imports as well.

          One question: since this is a metadat change, do we need to up the MetaDataVersion number, and add the version checks on deserialing digestType and metadat for more safe?

          @Sijie, It would be great if you take a look on it beased on your time?

          Show
          Uma Maheswara Rao G added a comment - nit: BookKeeperAdmin: one unused import for DigestType. as we are touching the file, I would suggest clean the imports as well. One question: since this is a metadat change, do we need to up the MetaDataVersion number, and add the version checks on deserialing digestType and metadat for more safe? @Sijie, It would be great if you take a look on it beased on your time?
          Hide
          Ivan Kelly added a comment -

          Updated to latest trunk.

          Show
          Ivan Kelly added a comment - Updated to latest trunk.
          Hide
          Ivan Kelly added a comment -

          The patch applied on top of BOOKKEEPER-303

          Show
          Ivan Kelly added a comment - The patch applied on top of BOOKKEEPER-303
          Hide
          Ivan Kelly added a comment -

          Patch is pretty straight forward. For all new ledgers, store the password and digest type. They can then be opened without having to specify the password using asyncOpenLedger on BookKeeperAdmin. I've only create async versions of the calls for now. We can add sync in future is people want them. This api isn't for general use, only from administrative tools.

          Show
          Ivan Kelly added a comment - Patch is pretty straight forward. For all new ledgers, store the password and digest type. They can then be opened without having to specify the password using asyncOpenLedger on BookKeeperAdmin. I've only create async versions of the calls for now. We can add sync in future is people want them. This api isn't for general use, only from administrative tools.
          Hide
          Flavio Junqueira added a comment -

          I just noticed that you had a comment here now, Dhruba. I was not really proposing anything, I was just pointing out that it didn't sound right to me to give keys to bookkeeper and expect it to manage it for the application. When I say manage, I mean having bookkeeper storing and using keys transparently for the application.

          Show
          Flavio Junqueira added a comment - I just noticed that you had a comment here now, Dhruba. I was not really proposing anything, I was just pointing out that it didn't sound right to me to give keys to bookkeeper and expect it to manage it for the application. When I say manage, I mean having bookkeeper storing and using keys transparently for the application.
          Hide
          dhruba borthakur added a comment -

          hi flavio, can you pl explain your proposal in a little more detail? (please pardon my ignorance).

          Show
          dhruba borthakur added a comment - hi flavio, can you pl explain your proposal in a little more detail? (please pardon my ignorance).
          Hide
          Flavio Junqueira added a comment -

          Why can't we assume that the key is provided as part of the application input and let the application manage keys directly? It doesn't seem to be a good design choice to have BK managing keys on behalf of the application.

          Show
          Flavio Junqueira added a comment - Why can't we assume that the key is provided as part of the application input and let the application manage keys directly? It doesn't seem to be a good design choice to have BK managing keys on behalf of the application.

            People

            • Assignee:
              Ivan Kelly
              Reporter:
              Benjamin Reed
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development