Skip to content

Conversation

@liketic
Copy link

@liketic liketic commented Jan 12, 2019

Migrate more classes to Writable.

Relates to #34389

@liketic liketic changed the title Migrate Streamable to Writable Migrate Streamable to Writeable Jan 12, 2019
@dnhatn
Copy link
Member

dnhatn commented Jan 13, 2019

@liketic Thanks for picking up this :). It would be easier to review if you migrate one class or one package per PR. Would you mind narrowing this PR to the "recovery" package only?

@liketic liketic force-pushed the migrate-Streamble-to-Writeable branch from 43c60bb to 2859ee8 Compare January 13, 2019 03:40
@liketic
Copy link
Author

liketic commented Jan 13, 2019

Thanks @dnhatn , 👍 I updated this as you requested. Could you please review again?

@liketic liketic changed the title Migrate Streamable to Writeable Migrate Streamable to Writeable for recovery package Jan 13, 2019
@dnhatn dnhatn added the :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. label Jan 13, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@dnhatn
Copy link
Member

dnhatn commented Jan 13, 2019

@liketic Thank you! I will have a look.
@elasticmachine ok to test.

@dnhatn
Copy link
Member

dnhatn commented Jan 13, 2019

@elasticmachine retest this please.

@dnhatn
Copy link
Member

dnhatn commented Jan 22, 2019

@elasticmachine retest this please.

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

Thanks again @liketic. I left some comments but this looks great. Would you please bring your PR up to date with the master branch?

@dnhatn
Copy link
Member

dnhatn commented Jan 25, 2019

Thanks @liketic. This is really good already but one more ask. Could you please move writeTo method next to the constructor so we scan them visually for differences ?

@liketic
Copy link
Author

liketic commented Jan 25, 2019

@dnhatn I updated as you requested. Please review again.

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

@liketic I pushed f08269f to arrange the ctors of RecoveryState. I'll wait for CI then pull this in. Thank you very much for your PR :).

@dnhatn dnhatn merged commit eb7bf16 into elastic:master Jan 25, 2019
@dnhatn dnhatn self-assigned this Jan 25, 2019
dnhatn pushed a commit that referenced this pull request Jan 25, 2019
@dnhatn
Copy link
Member

dnhatn commented Jan 25, 2019

I have merged this and backported to 6.x. Thanks again @liketic.

@dnhatn dnhatn removed their assignment Jan 25, 2019
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 25, 2019
* elastic/master: (68 commits)
  Fix potential IllegalCapacityException in LLRC when selecting nodes (elastic#37821)
  Mute CcrRepositoryIT#testFollowerMappingIsUpdated
  Fix S3 Repository ITs When Docker is not Available (elastic#37878)
  Pass distribution type through to docs tests (elastic#37885)
  Mute SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard
  SQL: Fix casting from date to numeric type to use millis (elastic#37869)
  Migrate o.e.i.r.RecoveryState to Writeable (elastic#37380)
  ML: removing unnecessary upgrade code (elastic#37879)
  Relax cluster metadata version check (elastic#37834)
  Mute TransformIntegrationTests#testSearchTransform
  Refactored GeoHashGrid unit tests (elastic#37832)
  Fixes for a few randomized agg tests that fail hasValue() checks
  Geo: replace intermediate geo objects with libs/geo (elastic#37721)
  Remove NOREPLACE for /etc/elasticsearch in rpm and deb (elastic#37839)
  Remove "reinstall" packaging tests (elastic#37851)
  Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor (elastic#37756)
  Exit batch files explictly using ERRORLEVEL (elastic#29583)
  TransportUnfollowAction should increase settings version (elastic#37859)
  AsyncTwoPhaseIndexerTests race condition fixed (elastic#37830)
  Do not allow negative variances (elastic#37384)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. >refactoring v6.7.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants