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

          Benjamin Reed created issue -
          Benjamin Reed made changes -
          Field Original Value New Value
          Link This issue relates to ZOOKEEPER-712 [ ZOOKEEPER-712 ]
          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.
          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).
          Benjamin Reed made changes -
          Project ZooKeeper [ 12310801 ] Bookkeeper [ 12311293 ]
          Key ZOOKEEPER-807 BOOKKEEPER-2
          Workflow no-reopen-closed, patch-avail [ 12515359 ] jira [ 12612002 ]
          Component/s contrib-bookkeeper [ 12312643 ]
          Gavin made changes -
          Workflow jira [ 12612002 ] no-reopen-closed, patch-avail [ 12614550 ]
          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.
          Ivan Kelly made changes -
          Fix Version/s 4.2.0 [ 12320244 ]
          Ivan Kelly made changes -
          Link This issue is duplicated by BOOKKEEPER-221 [ BOOKKEEPER-221 ]
          Ivan Kelly made changes -
          Parent BOOKKEEPER-237 [ 12553925 ]
          Issue Type Bug [ 1 ] Sub-task [ 7 ]
          Ivan Kelly made changes -
          Link This issue blocks BOOKKEEPER-247 [ BOOKKEEPER-247 ]
          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.
          Ivan Kelly made changes -
          Attachment BOOKKEEPER-2.diff [ 12532742 ]
          Ivan Kelly made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Assignee Ivan Kelly [ ikelly ]
          Ivan Kelly made changes -
          Link This issue requires BOOKKEEPER-303 [ BOOKKEEPER-303 ]
          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
          Ivan Kelly made changes -
          Link This issue blocks BOOKKEEPER-246 [ BOOKKEEPER-246 ]
          Hide
          Ivan Kelly added a comment -

          Updated to latest trunk.

          Show
          Ivan Kelly added a comment - Updated to latest trunk.
          Ivan Kelly made changes -
          Attachment BOOKKEEPER-2.diff [ 12534000 ]
          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
          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
          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.
          Ivan Kelly made changes -
          Attachment BOOKKEEPER-2.diff [ 12536049 ]
          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 new patch looks good to me. +1.

          Show
          Sijie Guo added a comment - the new patch looks good to me. +1.
          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
          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
          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
          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.
          Sijie Guo made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          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
          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.
          Ivan Kelly made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development