diff --git a/include/istio/mixerclient/options.h b/include/istio/mixerclient/options.h index cac81b392c4..f24a0a17f23 100644 --- a/include/istio/mixerclient/options.h +++ b/include/istio/mixerclient/options.h @@ -45,8 +45,8 @@ struct CheckOptions { // Options controlling report batch. struct ReportOptions { // Default constructor. - // Default to batch up to 1000 reports or 1 seconds. - ReportOptions() : max_batch_entries(1000), max_batch_time_ms(1000) {} + // Default to batch up to 500 reports or 1 seconds. + ReportOptions() : max_batch_entries(100), max_batch_time_ms(1000) {} // Constructor. ReportOptions(int max_batch_entries, int max_batch_time_ms) diff --git a/src/istio/mixerclient/BUILD b/src/istio/mixerclient/BUILD index a34cc53e6c5..eb589cf22d5 100644 --- a/src/istio/mixerclient/BUILD +++ b/src/istio/mixerclient/BUILD @@ -42,8 +42,6 @@ cc_library( "check_cache.h", "client_impl.cc", "client_impl.h", - "delta_update.cc", - "delta_update.h", "global_dictionary.cc", "global_dictionary.h", "quota_cache.cc", @@ -96,17 +94,6 @@ cc_test( ], ) -cc_test( - name = "delta_update_test", - size = "small", - srcs = ["delta_update_test.cc"], - linkstatic = 1, - deps = [ - ":mixerclient_lib", - "//external:googletest_main", - ], -) - cc_test( name = "report_batch_test", size = "small", diff --git a/src/istio/mixerclient/attribute_compressor.cc b/src/istio/mixerclient/attribute_compressor.cc index f60e6f26ea0..701c1672098 100644 --- a/src/istio/mixerclient/attribute_compressor.cc +++ b/src/istio/mixerclient/attribute_compressor.cc @@ -15,7 +15,6 @@ #include "src/istio/mixerclient/attribute_compressor.h" #include "include/istio/utils/protobuf.h" -#include "src/istio/mixerclient/delta_update.h" #include "src/istio/mixerclient/global_dictionary.h" using ::istio::mixer::v1::Attributes; @@ -77,10 +76,8 @@ ::istio::mixer::v1::StringMap CreateStringMap( return compressed_map; } -bool CompressByDict(const Attributes& attributes, MessageDictionary& dict, - DeltaUpdate& delta_update, CompressedAttributes* pb) { - delta_update.Start(); - +void CompressByDict(const Attributes& attributes, MessageDictionary& dict, + CompressedAttributes* pb) { // Fill attributes. for (const auto& it : attributes.attributes()) { const std::string& name = it.first; @@ -88,11 +85,6 @@ bool CompressByDict(const Attributes& attributes, MessageDictionary& dict, int index = dict.GetIndex(name); - // Check delta update. If same, skip it. - if (delta_update.Check(index, value)) { - continue; - } - // Fill the attribute to proper map. switch (value.value_case()) { case Attributes_AttributeValue::kStringValue: @@ -124,26 +116,19 @@ bool CompressByDict(const Attributes& attributes, MessageDictionary& dict, break; } } - - return delta_update.Finish(); } class BatchCompressorImpl : public BatchCompressor { public: BatchCompressorImpl(const GlobalDictionary& global_dict) - : dict_(global_dict), - delta_update_(DeltaUpdate::Create()), - report_(new ::istio::mixer::v1::ReportRequest) { + : dict_(global_dict), report_(new ::istio::mixer::v1::ReportRequest) { report_->set_global_word_count(global_dict.size()); } - bool Add(const Attributes& attributes) override { + void Add(const Attributes& attributes) override { CompressedAttributes pb; - if (!CompressByDict(attributes, dict_, *delta_update_, &pb)) { - return false; - } + CompressByDict(attributes, dict_, &pb); pb.GetReflection()->Swap(report_->add_attributes(), &pb); - return true; } int size() const override { return report_->attributes_size(); } @@ -157,7 +142,6 @@ class BatchCompressorImpl : public BatchCompressor { private: MessageDictionary dict_; - std::unique_ptr delta_update_; std::unique_ptr<::istio::mixer::v1::ReportRequest> report_; }; @@ -194,9 +178,7 @@ void AttributeCompressor::Compress( const Attributes& attributes, ::istio::mixer::v1::CompressedAttributes* pb) const { MessageDictionary dict(global_dict_); - std::unique_ptr delta_update = DeltaUpdate::CreateNoOp(); - - CompressByDict(attributes, dict, *delta_update, pb); + CompressByDict(attributes, dict, pb); for (const std::string& word : dict.GetWords()) { pb->add_words(word); diff --git a/src/istio/mixerclient/attribute_compressor.h b/src/istio/mixerclient/attribute_compressor.h index f37072f692b..8ba2e6b3107 100644 --- a/src/istio/mixerclient/attribute_compressor.h +++ b/src/istio/mixerclient/attribute_compressor.h @@ -50,8 +50,7 @@ class BatchCompressor { virtual ~BatchCompressor() {} // Add an attribute set to the batch. - // Return false if it could not be added for delta update. - virtual bool Add(const ::istio::mixer::v1::Attributes& attributes) = 0; + virtual void Add(const ::istio::mixer::v1::Attributes& attributes) = 0; // Get the batched size. virtual int size() const = 0; diff --git a/src/istio/mixerclient/attribute_compressor_test.cc b/src/istio/mixerclient/attribute_compressor_test.cc index 34fa59b3e67..721d2bb07bd 100644 --- a/src/istio/mixerclient/attribute_compressor_test.cc +++ b/src/istio/mixerclient/attribute_compressor_test.cc @@ -146,10 +146,22 @@ attributes { } } attributes { + strings { + key: 2 + value: 127 + } + strings { + key: 6 + value: 101 + } int64s { key: 1 value: 135 } + int64s { + key: 8 + value: 8080 + } int64s { key: 27 value: 111 @@ -162,6 +174,75 @@ attributes { key: 71 value: false } + timestamps { + key: 132 + value { + } + } + durations { + key: 29 + value { + seconds: 5 + } + } + bytes { + key: 0 + value: "text/html; charset=utf-8" + } + string_maps { + key: 15 + value { + entries { + key: 32 + value: 90 + } + entries { + key: 58 + value: 104 + } + } + } +} +attributes { + strings { + key: 2 + value: 127 + } + strings { + key: 6 + value: 101 + } + int64s { + key: 1 + value: 135 + } + int64s { + key: 8 + value: 8080 + } + doubles { + key: 78 + value: 123.99 + } + bools { + key: 71 + value: false + } + timestamps { + key: 132 + value { + } + } + durations { + key: 29 + value { + seconds: 5 + } + } + bytes { + key: 0 + value: "text/html; charset=utf-8" + } string_maps { key: 15 value { @@ -177,7 +258,7 @@ attributes { } } default_words: "JWT-Token" -global_word_count: 111 +global_word_count: 221 )"; class AttributeCompressorTest : public ::testing::Test { @@ -234,7 +315,7 @@ TEST_F(AttributeCompressorTest, BatchCompressTest) { AttributeCompressor compressor; auto batch_compressor = compressor.CreateBatchCompressor(); - EXPECT_TRUE(batch_compressor->Add(attributes_)); + batch_compressor->Add(attributes_); // modify some attributes utils::AttributesBuilder builder(&attributes_); @@ -245,13 +326,13 @@ TEST_F(AttributeCompressorTest, BatchCompressTest) { builder.AddStringMap("request.headers", {{"content-type", "application/json"}, {":method", "GET"}}); - // Since there is no deletion, batch is good - EXPECT_TRUE(batch_compressor->Add(attributes_)); + // Batch the second one with added attributes + batch_compressor->Add(attributes_); // remove a key attributes_.mutable_attributes()->erase("response.size"); - // Batch should fail. - EXPECT_FALSE(batch_compressor->Add(attributes_)); + // Batch the third with a removed attribute. + batch_compressor->Add(attributes_); auto report_pb = batch_compressor->Finish(); @@ -262,7 +343,7 @@ TEST_F(AttributeCompressorTest, BatchCompressTest) { ::istio::mixer::v1::ReportRequest expected_report_pb; ASSERT_TRUE( TextFormat::ParseFromString(kReportAttributes, &expected_report_pb)); - report_pb->set_global_word_count(111); + report_pb->set_global_word_count(221); EXPECT_TRUE(MessageDifferencer::Equals(*report_pb, expected_report_pb)); } diff --git a/src/istio/mixerclient/delta_update.cc b/src/istio/mixerclient/delta_update.cc deleted file mode 100644 index e4f02641671..00000000000 --- a/src/istio/mixerclient/delta_update.cc +++ /dev/null @@ -1,86 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * Licensed 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 "src/istio/mixerclient/delta_update.h" - -#include "google/protobuf/util/message_differencer.h" - -#include - -using ::google::protobuf::util::MessageDifferencer; -using ::istio::mixer::v1::Attributes_AttributeValue; - -namespace istio { -namespace mixerclient { -namespace { - -class DeltaUpdateImpl : public DeltaUpdate { - public: - // Start a update for a request. - void Start() override { - prev_set_.clear(); - for (const auto& it : prev_map_) { - prev_set_.insert(it.first); - } - } - - bool Check(int index, const Attributes_AttributeValue& value) override { - bool same = false; - const auto& it = prev_map_.find(index); - if (it != prev_map_.end()) { - if (MessageDifferencer::Equals(it->second, value)) { - same = true; - } - } - if (!same) { - prev_map_[index] = value; - } - prev_set_.erase(index); - return same; - } - - // "deleted" is not supported for now. If some attributes are missing, - // return false to indicate delta update is not supported. - bool Finish() override { return prev_set_.empty(); } - - private: - // The remaining attributes from previous. - std::set prev_set_; - - // The attribute map from previous. - std::map prev_map_; -}; - -// An optimization for non-delta update case. -class DeltaUpdateNoOpImpl : public DeltaUpdate { - public: - void Start() override {} - bool Check(int index, const Attributes_AttributeValue& value) override { - return false; - } - bool Finish() override { return true; } -}; - -} // namespace - -std::unique_ptr DeltaUpdate::Create() { - return std::unique_ptr(new DeltaUpdateImpl); -} - -std::unique_ptr DeltaUpdate::CreateNoOp() { - return std::unique_ptr(new DeltaUpdateNoOpImpl); -} - -} // namespace mixerclient -} // namespace istio diff --git a/src/istio/mixerclient/delta_update.h b/src/istio/mixerclient/delta_update.h deleted file mode 100644 index 3ec16a65f5d..00000000000 --- a/src/istio/mixerclient/delta_update.h +++ /dev/null @@ -1,59 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * Licensed 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. - */ - -#ifndef ISTIO_MIXERCLIENT_DELTA_UPDATE_H -#define ISTIO_MIXERCLIENT_DELTA_UPDATE_H - -#include "mixer/v1/attributes.pb.h" - -#include - -namespace istio { -namespace mixerclient { - -// A class to support attribute delta update. -// It has previous attribute values and check -// for the current one. -class DeltaUpdate { - public: - virtual ~DeltaUpdate() {} - - // Start a new delta update - virtual void Start() = 0; - - // Check an attribute, return true if it is in the previous - // set with same value, so no need to send it again. - // Each attribute in the current set needs to call this method. - virtual bool Check( - int index, - const ::istio::mixer::v1::Attributes_AttributeValue& value) = 0; - - // Finish a delta update. - // Return false if delta update is not supported. - // For example, "deleted" is not supported, if some attributes are - // missing, delta update will not be supported. - virtual bool Finish() = 0; - - // Create an instance. - static std::unique_ptr Create(); - - // Create an no-op instance; an optimization for no delta update cases. - static std::unique_ptr CreateNoOp(); -}; - -} // namespace mixerclient -} // namespace istio - -#endif // ISTIO_MIXERCLIENT_DELTA_UPDATE_H diff --git a/src/istio/mixerclient/delta_update_test.cc b/src/istio/mixerclient/delta_update_test.cc deleted file mode 100644 index b2fda95b297..00000000000 --- a/src/istio/mixerclient/delta_update_test.cc +++ /dev/null @@ -1,102 +0,0 @@ -/* Copyright 2017 Istio Authors. All Rights Reserved. - * - * Licensed 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 "src/istio/mixerclient/delta_update.h" -#include "gtest/gtest.h" - -using ::istio::mixer::v1::Attributes_AttributeValue; - -namespace istio { -namespace mixerclient { - -class DeltaUpdateTest : public ::testing::Test { - public: - void SetUp() { - string_map_value_ = StringMapValue({{"foo", "bar"}}); - update_ = DeltaUpdate::Create(); - - update_->Start(); - EXPECT_FALSE(update_->Check(1, Int64Value(1))); - EXPECT_FALSE(update_->Check(2, Int64Value(2))); - EXPECT_FALSE(update_->Check(3, string_map_value_)); - EXPECT_TRUE(update_->Finish()); - } - - ::istio::mixer::v1::Attributes_AttributeValue Int64Value(int64_t i) { - ::istio::mixer::v1::Attributes_AttributeValue v; - v.set_int64_value(i); - return v; - } - - ::istio::mixer::v1::Attributes_AttributeValue StringValue( - const std::string& str) { - ::istio::mixer::v1::Attributes_AttributeValue v; - v.set_string_value(str); - return v; - } - - ::istio::mixer::v1::Attributes_AttributeValue StringMapValue( - std::map&& string_map) { - ::istio::mixer::v1::Attributes_AttributeValue v; - auto entries = v.mutable_string_map_value()->mutable_entries(); - for (const auto& map_it : string_map) { - (*entries)[map_it.first] = map_it.second; - } - return v; - } - - std::unique_ptr update_; - Attributes_AttributeValue string_map_value_; -}; - -TEST_F(DeltaUpdateTest, TestUpdateNoDelete) { - update_->Start(); - // 1: value is the same. - EXPECT_TRUE(update_->Check(1, Int64Value(1))); - // 2: value is different. - EXPECT_FALSE(update_->Check(2, Int64Value(3))); - // 3: compare string map. - EXPECT_TRUE(update_->Check(3, string_map_value_)); - // 4: an new attribute. - EXPECT_FALSE(update_->Check(4, Int64Value(4))); - // No missing item - EXPECT_TRUE(update_->Finish()); -} - -TEST_F(DeltaUpdateTest, TestUpdateWithDelete) { - update_->Start(); - // 1: value is the same. - EXPECT_TRUE(update_->Check(1, Int64Value(1))); - - // 2: is missing - - // 3: compare string map - EXPECT_FALSE(update_->Check(3, StringMapValue({}))); - - // 4: an new attribute. - EXPECT_FALSE(update_->Check(4, Int64Value(4))); - - // There is a missing item - EXPECT_FALSE(update_->Finish()); -} - -TEST_F(DeltaUpdateTest, TestDifferentType) { - update_->Start(); - // 1 is differnt type. - EXPECT_FALSE(update_->Check(1, StringValue(""))); -} - -} // namespace mixerclient -} // namespace istio diff --git a/src/istio/mixerclient/report_batch.cc b/src/istio/mixerclient/report_batch.cc index a5a5a6c821b..c41da3b17a8 100644 --- a/src/istio/mixerclient/report_batch.cc +++ b/src/istio/mixerclient/report_batch.cc @@ -45,13 +45,7 @@ void ReportBatch::Report(const Attributes& request) { batch_compressor_ = compressor_.CreateBatchCompressor(); } - if (!batch_compressor_->Add(request)) { - FlushWithLock(); - - batch_compressor_ = compressor_.CreateBatchCompressor(); - batch_compressor_->Add(request); - } - + batch_compressor_->Add(request); if (batch_compressor_->size() >= options_.max_batch_entries) { FlushWithLock(); } else { diff --git a/src/istio/mixerclient/report_batch_test.cc b/src/istio/mixerclient/report_batch_test.cc index 8893d8e4535..9cfcfb72242 100644 --- a/src/istio/mixerclient/report_batch_test.cc +++ b/src/istio/mixerclient/report_batch_test.cc @@ -106,29 +106,6 @@ TEST_F(ReportBatchTest, TestBatchReport) { EXPECT_EQ(report_call_count, 4); } -TEST_F(ReportBatchTest, TestNoDeltaUpdate) { - int report_call_count = 0; - EXPECT_CALL(mock_report_transport_, Report(_, _, _)) - .WillRepeatedly(Invoke([&](const ReportRequest& request, - ReportResponse* response, DoneFunc on_done) { - report_call_count++; - on_done(Status::OK); - })); - - Attributes report; - utils::AttributesBuilder(&report).AddString("key", "value"); - batch_->Report(report); - EXPECT_EQ(report_call_count, 0); - - // Erase a key, so delta update fail to push the batched result. - report.mutable_attributes()->erase("key"); - batch_->Report(report); - EXPECT_EQ(report_call_count, 1); - - batch_->Flush(); - EXPECT_EQ(report_call_count, 2); -} - TEST_F(ReportBatchTest, TestBatchReportWithTimeout) { int report_call_count = 0; EXPECT_CALL(mock_report_transport_, Report(_, _, _))