trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Igor Galić <i.ga...@brainsware.org>
Subject Re: git commit: TS-1678: Simplify register_record()
Date Fri, 01 Feb 2013 18:36:27 GMT
On Friday, February 01, 2013 04:36:37 PM jpeach@apache.org wrote:
> Updated Branches:
>   refs/heads/master baca4d2b7 -> 5f93bc50f
> 
> 
> TS-1678: Simplify register_record()
> 
> 'release_record_lock' parameter to register_record() is unnecessarily
> complicated and the lock release is dangerous. Let's simplify it

I'm all for removing code but would you care to explain why "the lock release 
is dangerous" ?

-- i

> and remove verbose lock releasing from the upper layer code.
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/trafficserver/repo
> Commit: http://git-wip-us.apache.org/repos/asf/trafficserver/commit/5f93bc50
> Tree: http://git-wip-us.apache.org/repos/asf/trafficserver/tree/5f93bc50
> Diff: http://git-wip-us.apache.org/repos/asf/trafficserver/diff/5f93bc50
> 
> Branch: refs/heads/master
> Commit: 5f93bc50fd1869f4eb2866ecffd92fc616835fb4
> Parents: baca4d2
> Author: Yunkai Zhang <qiushu.zyk@taobao.com>
> Authored: Fri Feb 1 08:35:07 2013 -0800
> Committer: James Peach <jpeach@apache.org>
> Committed: Fri Feb 1 08:35:07 2013 -0800
> 
> ----------------------------------------------------------------------
>  CHANGES                |    3 +++
>  lib/records/RecCore.cc |   18 +++---------------
>  2 files changed, 6 insertions(+), 15 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5f93bc50/CHANGES
> ----------------------------------------------------------------------
> diff --git a/CHANGES b/CHANGES
> index ad32f87..9da2ed4 100644
> --- a/CHANGES
> +++ b/CHANGES
> @@ -1,6 +1,9 @@
>                                                           -*- coding: utf-8
> -*- Changes with Apache Traffic Server 3.3.1
> 
> +  *) [TS-1678] Simplify register_record
> +     Author: Yunkai Zhang <qiushu.zyk@taobao.com>
> +
>    *) [TS-1252] stats summary in cluster not working
>       Author: Yunkai Zhang <qiushu.zyk@taobao.com>
> 
> 
> http://git-wip-us.apache.org/repos/asf/trafficserver/blob/5f93bc50/lib/recor
> ds/RecCore.cc
> ---------------------------------------------------------------------- diff
> --git a/lib/records/RecCore.cc b/lib/records/RecCore.cc
> index d9215ed..4888fbb 100644
> --- a/lib/records/RecCore.cc
> +++ b/lib/records/RecCore.cc
> @@ -52,14 +52,11 @@ RecTree *g_records_tree = NULL;
>  // register_record
>  //-------------------------------------------------------------------------
> static RecRecord *
> -register_record(RecT rec_type, const char *name, RecDataT data_type,
> RecData data_default, bool release_record_lock) +register_record(RecT
> rec_type, const char *name, RecDataT data_type, RecData data_default) {
>    RecRecord *r = NULL;
> 
>    if (ink_hash_table_lookup(g_records_ht, name, (void **) &r)) {
> -    if (release_record_lock) {
> -      rec_mutex_acquire(&(r->lock));
> -    }
>      ink_release_assert(r->rec_type == rec_type);
>      ink_release_assert(r->data_type == data_type);
>      // Note: do not set r->data as we want to keep the previous value
> @@ -68,9 +65,6 @@ register_record(RecT rec_type, const char *name, RecDataT
> data_type, RecData dat if ((r = RecAlloc(rec_type, name, data_type)) ==
> NULL) {
>        return NULL;
>      }
> -    if (release_record_lock) {
> -      rec_mutex_acquire(&(r->lock));
> -    }
>      // Set the r->data to its default value as this is a new record
>      RecDataSet(r->data_type, &(r->data), &(data_default));
>      RecDataSet(r->data_type, &(r->data_default), &(data_default));
> @@ -80,10 +74,6 @@ register_record(RecT rec_type, const char *name, RecDataT
> data_type, RecData dat // we're now registered
>    r->registered = true;
> 
> -  if (release_record_lock) {
> -    rec_mutex_release(&(r->lock));
> -  }
> -
>    return r;
>  }
> 
> @@ -733,9 +723,8 @@ RecRegisterStat(RecT rec_type, const char *name,
> RecDataT data_type, RecData dat RecRecord *r = NULL;
> 
>    ink_rwlock_wrlock(&g_records_rwlock);
> -  if ((r = register_record(rec_type, name, data_type, data_default, false))
> != NULL) { +  if ((r = register_record(rec_type, name, data_type,
> data_default)) != NULL) { r->stat_meta.persist_type = persist_type;
> -    rec_mutex_release(&(r->lock));
>    } else {
>      ink_debug_assert(!"Can't register record!");
>    }
> @@ -755,7 +744,7 @@ RecRegisterConfig(RecT rec_type, const char *name,
> RecDataT data_type, {
>    RecRecord *r;
>    ink_rwlock_wrlock(&g_records_rwlock);
> -  if ((r = register_record(rec_type, name, data_type, data_default, false))
> != NULL) { +  if ((r = register_record(rec_type, name, data_type,
> data_default)) != NULL) { // Note: do not modify
> 'record->config_meta.update_required'
>      r->config_meta.update_type = update_type;
>      r->config_meta.check_type = check_type;
> @@ -765,7 +754,6 @@ RecRegisterConfig(RecT rec_type, const char *name,
> RecDataT data_type, r->config_meta.check_expr = ats_strdup(check_expr);
>      r->config_meta.update_cb_list = NULL;
>      r->config_meta.access_type = access_type;
> -    rec_mutex_release(&(r->lock));
>    }
>    ink_rwlock_unlock(&g_records_rwlock);

Mime
View raw message