{"action":"created","issue":{"url":"https://api.github.com/repos/apache/bookkeeper/issues/1283","repository_url":"https://api.github.com/repos/apache/bookkeeper","labels_url":"https://api.github.com/repos/apache/bookkeeper/issues/1283/labels{/name}","comments_url":"https://api.github.com/repos/apache/bookkeeper/issues/1283/comments","events_url":"https://api.github.com/repos/apache/bookkeeper/issues/1283/events","html_url":"https://github.com/apache/bookkeeper/pull/1283","id":307448584,"number":1283,"title":"Issue #1282: call to appendLedgersMap in flushCompactionLog","user":{"login":"reddycharan","id":13989266,"avatar_url":"https://avatars3.githubusercontent.com/u/13989266?v=4","gravatar_id":"","url":"https://api.github.com/users/reddycharan","html_url":"https://github.com/reddycharan","followers_url":"https://api.github.com/users/reddycharan/followers","following_url":"https://api.github.com/users/reddycharan/following{/other_user}","gists_url":"https://api.github.com/users/reddycharan/gists{/gist_id}","starred_url":"https://api.github.com/users/reddycharan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/reddycharan/subscriptions","organizations_url":"https://api.github.com/users/reddycharan/orgs","repos_url":"https://api.github.com/users/reddycharan/repos","events_url":"https://api.github.com/users/reddycharan/events{/privacy}","received_events_url":"https://api.github.com/users/reddycharan/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2018-03-21T23:03:58Z","updated_at":"2018-03-27T18:10:34Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/apache/bookkeeper/pulls/1283","html_url":"https://github.com/apache/bookkeeper/pull/1283","diff_url":"https://github.com/apache/bookkeeper/pull/1283.diff","patch_url":"https://github.com/apache/bookkeeper/pull/1283.patch"},"body":"Descriptions of the changes in this PR:\r\n\r\nwith this change https://github.com/apache/bookkeeper/commit/3392beee5c70abe36d36604723e14d97b7764be9 compactionlog was introduced. But for compactionlog call to appendLedgersMap (// Append ledgers map at the end of entry log) isn't made.\r\n\r\nSo for these logs, getEntryLogMetadata call has to call extractEntryLogMetadataByScanning for getting the metadata of entrylog. Which is a perf hit.\r\n\r\nMaster Issue: #1282 "},"comment":{"url":"https://api.github.com/repos/apache/bookkeeper/issues/comments/376622093","html_url":"https://github.com/apache/bookkeeper/pull/1283#issuecomment-376622093","issue_url":"https://api.github.com/repos/apache/bookkeeper/issues/1283","id":376622093,"user":{"login":"sijie","id":1217863,"avatar_url":"https://avatars2.githubusercontent.com/u/1217863?v=4","gravatar_id":"","url":"https://api.github.com/users/sijie","html_url":"https://github.com/sijie","followers_url":"https://api.github.com/users/sijie/followers","following_url":"https://api.github.com/users/sijie/following{/other_user}","gists_url":"https://api.github.com/users/sijie/gists{/gist_id}","starred_url":"https://api.github.com/users/sijie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sijie/subscriptions","organizations_url":"https://api.github.com/users/sijie/orgs","repos_url":"https://api.github.com/users/sijie/repos","events_url":"https://api.github.com/users/sijie/events{/privacy}","received_events_url":"https://api.github.com/users/sijie/received_events","type":"User","site_admin":false},"created_at":"2018-03-27T18:10:34Z","updated_at":"2018-03-27T18:10:34Z","author_association":"CONTRIBUTOR","body":"@reddycharan \r\n\r\n>  I'm not sure what you meant by \"What if we flush the log channel successfully but not the ledger map?\". In which case can it happen? Are you saying that this scenario is not handled completely?\r\n\r\n so @yzang was asking if the flush ordering at line 494\r\n```\r\n             // flush the internal buffer back to filesystem but not sync disk\r\n             // so the readers could access the data from filesystem.\t\r\n             logChannel.flush();\t             logChannel.flush();\r\n \t \r\n             // Append ledgers map at the end of entry log\r\n             appendLedgersMap(logChannel);\r\n```\r\n\r\n@yzang : the `flush` here is writing all the buffer data back to filesystem. it is *NOT* flushing to disk. so the order at line 494 is correct: we write the buffer back to filesystem and append the ledger map after that. then checkpoint logic will fsync the entry log file (via flushAndForceWrite) (which happens at flushCurrentLog and flushRotatedLogs).\r\n\r\n\r\n> Also shall we move line 494 to before line 841 instead? It makes more sense to append the ledger map in flush function than when creating a new log?\r\n\r\n@yzang \r\n\r\n`flushRotatedLogs` only happens when checkpointing happen. The reason we put `appendLedgerMap()` at line 494 instead of before line 841 is a performance consideration. you can append ledger map into filesystem without fsync, so between you append ledger map and the real checkpoint happen, filesystem might already 'pdflush' the dirty pages back to disk. when the real fsync happen, there is no blocking I/O happening.\r\n\r\n@reddycharan @yzang hope this clarify the questions.\r\n\r\n\r\n\r\n\r\n\r\n"}}
{"actor":{"display_login":"sijie"}}