Skip to content

15234 take2 review2#1425

Closed
ekaterinadimitrova2 wants to merge 23 commits intoapache:trunkfrom
ekaterinadimitrova2:15234-take2-review2
Closed

15234 take2 review2#1425
ekaterinadimitrova2 wants to merge 23 commits intoapache:trunkfrom
ekaterinadimitrova2:15234-take2-review2

Conversation

@ekaterinadimitrova2
Copy link
Contributor

No description provided.

if (value.symbol.equalsIgnoreCase(symbol))
return value;
}
throw new IllegalArgumentException(String.format("Unsupported data rate unit: %s. Supported units are: %s",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelsembwever I will change this exception also to ConfigurationException on commit, I missed it

public int hinted_handoff_throttle_in_kb = 1024;
public int batchlog_replay_throttle_in_kb = 1024;
@Replaces(oldName = "hinted_handoff_throttle_in_kb", converter = Converters.KIBIBYTES_DATASTORAGE, deprecated = true)
public volatile DataStorageSpec hinted_handoff_throttle = new DataStorageSpec("1024KiB");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not missing something those should stay not volatile. I changed pre-commit

public long row_cache_size_in_mb = 0;
public volatile int row_cache_save_period = 0;
@Replaces(oldName = "row_cache_size_in_mb", converter = Converters.MEBIBYTES_DATA_STORAGE, deprecated = true)
public volatile DataStorageSpec row_cache_size = new DataStorageSpec("0MiB");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need of volatile again

public volatile Long index_summary_capacity_in_mb;
public volatile int index_summary_resize_interval_in_minutes = 60;
@Replaces(oldName = "index_summary_capacity_in_mb", converter = Converters.MEBIBYTES_DATA_STORAGE, deprecated = true)
public DataStorageSpec index_summary_capacity;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here volatile was needed. Adding pre-commit


long seconds;
//if the provided string value is just a number, then we create a Duration Spec value in seconds
if (matcher.find())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong function, we should use, I changed it pre-commit:
```
if (matcher.matches())
{
seconds = Long.parseLong(value);
return new DurationSpec(seconds, TimeUnit.SECONDS);
}

@ekaterinadimitrova2
Copy link
Contributor Author

@michaelsembwever This work was squashed and committed almost two weeks ago, should I just close this pull request? I am not sure what is the process as normally we use pull requests against our own forks during review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants