From 05aa4191bf8f34dc0d58f44993be42fca2547634 Mon Sep 17 00:00:00 2001 From: Elliott Clark Date: Fri, 22 Apr 2016 22:21:45 -0700 Subject: [PATCH] HBASE-15696 Move region location cache serialization into serde Summary: Use IOBuf for zk Test Plan: Added a unit test Differential Revision: https://reviews.facebook.net/D57147 --- hbase-native-client/core/location-cache.cc | 44 ++------ hbase-native-client/serde/BUCK | 10 ++ hbase-native-client/serde/zk-deserializer-test.cc | 129 ++++++++++++++++++++++ hbase-native-client/serde/zk-deserializer.cc | 78 +++++++++++++ hbase-native-client/serde/zk-deserializer.h | 35 ++++++ 5 files changed, 264 insertions(+), 32 deletions(-) create mode 100644 hbase-native-client/serde/zk-deserializer-test.cc create mode 100644 hbase-native-client/serde/zk-deserializer.cc create mode 100644 hbase-native-client/serde/zk-deserializer.h diff --git a/hbase-native-client/core/location-cache.cc b/hbase-native-client/core/location-cache.cc index 52e86e3..118d3e7 100644 --- a/hbase-native-client/core/location-cache.cc +++ b/hbase-native-client/core/location-cache.cc @@ -16,10 +16,12 @@ * limitations under the License. * */ -#include "location-cache.h" +#include "core/location-cache.h" #include +#include +#include "serde/zk-deserializer.h" #include "if/ZooKeeper.pb.h" using namespace std; @@ -66,45 +68,23 @@ void LocationCache::RefreshMetaLocation() { } ServerName LocationCache::ReadMetaLocation() { - char contents[4096]; + auto buf = IOBuf::create(4096); + ZkDeserializer derser; + // This needs to be int rather than size_t as that's what ZK expects. - int len = sizeof(contents); + int len = buf->capacity(); // TODO(elliott): handle disconnects/reconntion as needed. - int zk_result = zoo_get(this->zk_, META_LOCATION, 0, contents, &len, nullptr); + int zk_result = zoo_get(this->zk_, META_LOCATION, 0, reinterpret_cast(buf->writableData()), &len, nullptr); + LOG(ERROR) << "len = " << len; if (zk_result != ZOK || len < 9) { LOG(ERROR) << "Error getting meta location."; throw runtime_error("Error getting meta location"); } - // There should be a magic number for recoverable zk - if (static_cast(contents[0]) != 255) { - LOG(ERROR) << "Magic number not in ZK znode data expected 255 got =" - << unsigned(static_cast(contents[0])); - throw runtime_error("Magic number not in znode data"); - } - // pos will keep track of skipped bytes. - int pos = 1; - // How long is the id? - int id_len = 0; - for (int i = 0; i < 4; ++i) { - id_len = id_len << 8; - id_len = id_len | static_cast(contents[pos]); - ++pos; - } - // Skip the id - pos += id_len; - // Then all protobuf's for HBase are prefixed with a magic string. - // PBUF, so we skip that. - // TODO(eclark): check to make sure that the magic string is correct - // though I am not sure that will get us much. - pos += 4; + buf->append(len); MetaRegionServer mrs; - // Try to decode the protobuf. - // If there's an error bail out. - if (mrs.ParseFromArray(contents + pos, len - pos) == false) { - LOG(ERROR) << "Error parsing Protobuf Message"; - throw runtime_error("Error parsing protobuf"); + if (derser.parse(buf.get(), &mrs) == false) { + LOG(ERROR) << "Unable to decode"; } - return mrs.server(); } diff --git a/hbase-native-client/serde/BUCK b/hbase-native-client/serde/BUCK index 207607f..0014c0b 100644 --- a/hbase-native-client/serde/BUCK +++ b/hbase-native-client/serde/BUCK @@ -19,10 +19,12 @@ cxx_library(name="serde", exported_headers=[ "client-serializer.h", "client-deserializer.h", + "zk-deserializer.h", ], srcs=[ "client-serializer.cc", "client-deserializer.cc", + "zk-deserializer.cc", ], deps=[ "//if:if", @@ -52,3 +54,11 @@ cxx_test(name="client-deserializer-test", ":serde", "//if:if", ], ) +cxx_test(name="zk-deserializer-test", + srcs=[ + "zk-deserializer-test.cc", + ], + deps=[ + ":serde", + "//if:if", + ], ) diff --git a/hbase-native-client/serde/zk-deserializer-test.cc b/hbase-native-client/serde/zk-deserializer-test.cc new file mode 100644 index 0000000..7968576 --- /dev/null +++ b/hbase-native-client/serde/zk-deserializer-test.cc @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "serde/zk-deserializer.h" + +#include +#include +#include +#include + +#include "if/ZooKeeper.pb.h" + +using namespace hbase; +using namespace hbase::pb; +using namespace folly; +using namespace std; +using namespace folly::io; + +// Test that would test if there's nothing there. +TEST(TestZkDesializer, TestThrowNoMagicNum) { + ZkDeserializer deser; + MetaRegionServer mrs; + + + auto buf = IOBuf::create(100); + buf->append(100); + RWPrivateCursor c{buf.get()}; + c.write(99); + ASSERT_THROW(deser.parse(buf.get(), &mrs), runtime_error); +} + +// Test if the protobuf is in a format that we can't decode +TEST(TestZkDesializer, TestBadProtoThrow) { + ZkDeserializer deser; + MetaRegionServer mrs; + string magic{"PBUF"}; + + + // Set ServerName + mrs.mutable_server()->set_host_name("test"); + mrs.mutable_server()->set_port(567); + mrs.mutable_server()->set_start_code(9567); + + + // One byte magic number + // four bytes for id length + // four bytes for id + // four bytes for PBUF + uint32_t start_len = 1 + 4 + 4 + 4; + // How large the protobuf will be + uint32_t pbuf_size = mrs.ByteSize(); + + auto buf = IOBuf::create(start_len + pbuf_size); + buf->append(start_len + pbuf_size); + RWPrivateCursor c{buf.get()}; + + // Write the magic number + c.write(255); + // Write the id len + c.writeBE(4); + // Write the id + c.write(13); + // Write the PBUF string + c.push(reinterpret_cast(magic.c_str()), 4); + + // Create the protobuf + MetaRegionServer out; + ASSERT_THROW(deser.parse(buf.get(), &out), runtime_error); +} + +// Test to make sure the whole thing works. +TEST(TestZkDesializer, TestNoThrow) { + ZkDeserializer deser; + MetaRegionServer mrs; + string magic{"PBUF"}; + + + // Set ServerName + mrs.mutable_server()->set_host_name("test"); + mrs.mutable_server()->set_port(567); + mrs.mutable_server()->set_start_code(9567); + + + // One byte magic number + // four bytes for id length + // four bytes for id + // four bytes for PBUF + uint32_t start_len = 1 + 4 + 4 + 4; + // How large the protobuf will be + uint32_t pbuf_size = mrs.ByteSize(); + + auto buf = IOBuf::create(start_len + pbuf_size); + buf->append(start_len + pbuf_size); + RWPrivateCursor c{buf.get()}; + + // Write the magic number + c.write(255); + // Write the id len + c.writeBE(4); + // Write the id + c.write(13); + // Write the PBUF string + c.push(reinterpret_cast(magic.c_str()), 4); + + // Now write the serialized protobuf + mrs.SerializeWithCachedSizesToArray(buf->writableData() + start_len); + + + // Create the protobuf + MetaRegionServer out; + ASSERT_TRUE(deser.parse(buf.get(), &out)); + ASSERT_EQ(mrs.server().host_name(), out.server().host_name()); +} diff --git a/hbase-native-client/serde/zk-deserializer.cc b/hbase-native-client/serde/zk-deserializer.cc new file mode 100644 index 0000000..33cf809 --- /dev/null +++ b/hbase-native-client/serde/zk-deserializer.cc @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "serde/zk-deserializer.h" + +#include +#include +#include + +using hbase::ZkDeserializer; +using std::runtime_error; +using folly::IOBuf; +using folly::io::Cursor; +using google::protobuf::Message; + +static const std::string MAGIC_STRING = "PBUF"; + +bool ZkDeserializer::parse(IOBuf *buf, Message *out) { + + // The format is like this + // 1 byte of magic number. 255 + // 4 bytes of id length. + // id_length number of bytes for the id of who put up the znode + // 4 bytes of a magic string PBUF + // Then the protobuf serialized without a varint header. + + Cursor c{buf}; + + // There should be a magic number for recoverable zk + uint8_t magic_num = c.read(); + if (magic_num != 255) { + LOG(ERROR) << "Magic number not in ZK znode data expected 255 got =" + << unsigned(magic_num); + throw runtime_error("Magic number not in znode data"); + } + // How long is the id? + uint32_t id_len = c.readBE(); + + if (id_len >= c.length()) { + LOG(ERROR) << "After skiping the if from zookeeper data there's not enough " + "left to read anything else"; + throw runtime_error("Not enough bytes to decode from zookeeper"); + } + + // Skip the id + c.skip(id_len); + + // Make sure that the magic string is there. + if (MAGIC_STRING != c.readFixedString(4)) { + LOG(ERROR) << "There was no PBUF magic string."; + throw runtime_error("No PBUF magic string in the zookpeeper data."); + } + + // Try to decode the protobuf. + // If there's an error bail out. + if (out->ParseFromArray(c.data(), c.length()) == false) { + LOG(ERROR) << "Error parsing Protobuf Message"; + throw runtime_error("Error parsing protobuf"); + } + + return true; +} diff --git a/hbase-native-client/serde/zk-deserializer.h b/hbase-native-client/serde/zk-deserializer.h new file mode 100644 index 0000000..aa91661 --- /dev/null +++ b/hbase-native-client/serde/zk-deserializer.h @@ -0,0 +1,35 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ +#pragma once + +namespace google { +namespace protobuf { +class Message; +} +} +namespace folly { +class IOBuf; +} + +namespace hbase { +class ZkDeserializer { +public: + bool parse(folly::IOBuf *buf, google::protobuf::Message *out); +}; +} // namespace hbase -- 2.8.0-rc2