It's a good suggestion, and I've been thinking about how to best handle
this. Honestly, the right answer might be to do git rebase on master from
the PR branch rather than a merge. That might avoid this situation
altogether. Of course, that also comes with all the obligatory warnings
about rebasing publicly shared branches, and that anyone how has their own
copy will get conflicts. But I think that risk is probably minimal. I'll
put something together that covers both options along with the problems and
solutions for each. Hopefully that will make future collaboration that fits
this pattern easier for people.
On Fri, Nov 16, 2018, 5:42 AM Otto Fowler <ottobackwa...@gmail.com wrote:
>
> Maybe this is worth a confluence entry, not as a guide, but just to
> document what you did.
>
> On November 15, 2018 at 19:07:40, Michael Miklavcic (
> michael.miklav...@gmail.com) wrote:
>
> Ok, this is finally merged! Whew!
>
> Here's how I polished up the history at the end. I used other feature
> branch merges as a guideline around commit messaging.
>
> * fcd644ca 2018-11-15 | METRON-1834: Migrate Elasticsearch from
> TransportClient to new Java REST API (mmiklavc via mmiklavc) (HEAD ->
> master, origin/master, origin/HEAD, master-merge) [mmiklavc]
> |\
> | * 8bf3b6ec 2018-11-15 | METRON-1834: Migrate Elasticsearch from
> TransportClient to new Java REST API (mmiklavc via mmiklavc) closes
> apache/metron#1242 (stella-es-base2) [mmiklavc]
> | * e7e19fbb 2018-10-08 | METRON-1834: Migrate Elasticsearch from
> TransportClient to new Java REST API (cstella via mmiklavc) [cstella]
> * | 0c4c622b 2018-11-14 | METRON-1749 Update Angular to latest release in
> Management UI (sardell via nickwallen) closes apache/metron#1217 [sardell]
>
> On Thu, Nov 15, 2018 at 4:29 PM Michael Miklavcic <
> michael.miklav...@gmail.com> wrote:
>
>> Absolutely, that's part of what I did to validate. This output below also
>> exactly matches the diff I get when I run it from the raw PR branch.
>>
>> git diff master --stat
>> Upgrading.md
>> | 7 +++
>> dependencies_with_url.csv
>> | 2 +
>> metron-deployment/Kerberos-manual-setup.md
>> | 154
>> ++---------------------------------------------------
>>
>> metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/configuration/metron-env.xml
>> | 9 ----
>>
>> metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/metron_service.py
>> | 2 -
>>
>> metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/scripts/params/params_linux.py
>> | 3 +-
>>
>> metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/themes/metron_theme.json
>> | 10 ----
>>
>> metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/MetaAlertServiceImpl.java
>> | 2 +-
>> metron-platform/elasticsearch-shaded/pom.xml
>> | 47
>> +++-------------
>>
>> metron-platform/elasticsearch-shaded/src/main/resources/META-INF/log4j-provider.properties
>> | 18 -------
>> metron-platform/metron-common/README.md
>> | 48
>> +++++++++--------
>> metron-platform/metron-common/src/main/config/zookeeper/global.json
>> | 1 -
>>
>> metron-platform/metron-common/src/main/java/org/apache/metron/common/configuration/ConfigOption.java
>> | 7 +++
>> metron-platform/metron-elasticsearch/README.md
>> | 45
>> +++++++++++++++-
>> metron-platform/metron-elasticsearch/pom.xml
>> | 32
>> +++++------
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/client/ElasticsearchClient.java
>> | 245
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/client/ElasticsearchClientFactory.java
>> | 189
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/config/ElasticsearchClientConfig.java
>> | 187
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/config/ElasticsearchClientOptions.java
>> | 60 +++++++++++++++++++++
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchColumnMetadataDao.java
>> | 101 ++++++++++++++---------------------
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchDao.java
>> | 21 ++++----
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchMetaAlertDao.java
>> | 2 +-
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchMetaAlertSearchDao.java
>> | 6 ++-
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchMetaAlertUpdateDao.java
>> | 4 +-
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchRequestSubmitter.java
>> | 13 ++---
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchRetrieveLatestDao.java
>> | 28 +++++-----
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchSearchDao.java
>> | 19 ++-----
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDao.java
>> | 19 +++----
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/utils/ElasticsearchUtils.java
>> | 182
>> +++++++-------------------------------------------------------
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/utils/FieldMapping.java
>> | 32 +++++++++++
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/utils/FieldProperties.java
>> | 36 +++++++++++++
>>
>> metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/writer/ElasticsearchWriter.java
>> | 26 ++++-----
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchColumnMetadataDaoTest.java
>> | 59 +++++++++-----------
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchDaoTest.java
>> | 8 ++-
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchRequestSubmitterTest.java
>> | 23 ++++----
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/dao/ElasticsearchUpdateDaoTest.java
>> | 12 +++--
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/integration/ElasticsearchMetaAlertIntegrationTest.java
>> | 9 ++--
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/integration/ElasticsearchSearchIntegrationTest.java
>> | 144 +++++++++++++++++++++++++++++--------------------
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/integration/ElasticsearchUpdateIntegrationTest.java
>> | 2 +-
>>
>> metron-platform/metron-elasticsearch/src/test/java/org/apache/metron/elasticsearch/integration/components/ElasticSearchComponent.java
>> | 6 ++-
>>
>> metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/metaalert/MetaAlertSearchDao.java
>> | 4 +-
>>
>> metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/metaalert/MetaAlertIntegrationTest.java
>> | 2 +-
>>
>> metron-platform/metron-integration-test/src/main/config/zookeeper/global.json
>> | 2 +-
>> 43 files changed, 1134 insertions(+), 694 deletions(-)
>> (END)
>>
>> Incidentally, (adding this note for posterity) one of the other weird
>> results I saw when I did the diff after getting the squashing/merging
>> working was one duplicated method in an integration test. It was the only
>> example of this. Not sure why, but I removed the extra method manually and
>> everything looks good.
>>
>> 169 /**
>> 170 * Add test fields to a template with defined types in case they
>> are not defined in the sensor template shipped with Metron.
>> 171 * This is useful for testing certain cases, for example faceting
>> on fields of various types.
>> 172 * Template follows this pattern:
>> 173 * { "mappings" : { "xxx_doc" : { "properties" : { ... }}}}
>> 174 * @param template - this method has side effects - template is
>> modified with field mappings.
>> 175 * @param docType
>> 176 */
>> 177 private static void addTestFieldMappings(JSONObject template,
>> String docType) {
>> 178 Map mappings = (Map) template.get("mappings");
>> 179 Map docTypeJSON = (Map) mappings.get(docType);
>> 180 Map properties = (Map) docTypeJSON.get("properties");
>> 181 Map<String, String> longType = new HashMap<>();
>> 182 longType.put("type", "long");
>> 183 properties.put("long_field", longType);
>> 184 Map<String, String> floatType = new HashMap<>();
>> 185 floatType.put("type", "float");
>> 186 properties.put("latitude", floatType);
>> 187 Map<String, String> doubleType = new HashMap<>();
>> 188 doubleType.put("type", "double");
>> 189 properties.put("score", doubleType);
>> 190 }
>> 191
>> 192 /**
>> 193 * Add test fields to a template with defined types in case they
>> are not defined in the sensor template shipped with Metron.
>> 194 * This is useful for testing certain cases, for example faceting
>> on fields of various types.
>> 195 * @param template
>> 196 * @param docType
>> 197 */
>> 198 private static void addTestFieldMappings(JSONObject template,
>> String docType) {
>> 199 Map mappings = (Map) template.get("mappings");
>> 200 Map docTypeJSON = (Map) mappings.get(docType);
>> 201 Map properties = (Map) docTypeJSON.get("properties");
>> 202 Map<String, String> longType = new HashMap<>();
>> 203 longType.put("type", "long");
>> 204 properties.put("long_field", longType);
>> 205 Map<String, String> floatType = new HashMap<>();
>> 206 floatType.put("type", "float");
>> 207 properties.put("latitude", floatType);
>> 208 Map<String, String> doubleType = new HashMap<>();
>> 209 doubleType.put("type", "double");
>> 210 properties.put("score", doubleType);
>> 211 }
>>
>>
>> On Thu, Nov 15, 2018 at 4:21 PM Otto Fowler <ottobackwa...@gmail.com>
>> wrote:
>>
>>> Can you diff the trees to be sure?
>>>
>>>
>>> On November 15, 2018 at 17:52:40, Michael Miklavcic (
>>> michael.miklav...@gmail.com) wrote:
>>>
>>> So amazingly, this still has results in conflicts, but I am able to
>>> resolve
>>> them manually in a sensible fashion.
>>> git merge -X theirs es-rebased
>>> CONFLICT (rename/rename): Rename
>>>
>>> "metron-interface/metron-config/src/app/rxjs-operators.ts"->"metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/ParserRunnerResults.java"
>>> in branch "HEAD" rename
>>>
>>> "metron-interface/metron-config/src/app/rxjs-operators.ts"->"metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/utils/FieldMapping.java"
>>> in "stella-es-base2"
>>>
>>> So where I landed gives a history like the following:
>>> * df1195aa 2018-11-15 | METRON-1834: Migrate Elasticsearch from
>>> TransportClient to new Java REST API (mmiklavc via mmiklavc) closes
>>> apache/metron#1242 (HEAD -> master-merge) [mmiklavc]
>>> |\
>>> | * 590b3669 2018-11-15 | METRON-1834: Migrate Elasticsearch from
>>> TransportClient to new Java REST API (mmiklavc via mmiklavc) closes
>>> apache/metron#1242 (stella-es-base2) [mmiklavc]
>>> | * a7c7dc28 2018-10-08 | Casey Stella - elasticsearch rest client
>>> migration base work (stella-es-base) [cstella]
>>> * | 0c4c622b 2018-11-14 | METRON-1749 Update Angular to latest release in
>>> Management UI (sardell via nickwallen) closes apache/metron#1217
>>> (origin/master, origin/HEAD, master) [sardell]
>>> ...
>>>
>>> I can modify a7c7dc28 commit message as well and hopefully this will be
>>> good for everyone?
>>>
>>> Cheers,
>>> Mike
>>>
>>>
>>>
>>>
>>> On Thu, Nov 15, 2018 at 2:29 PM Justin Leet <justinjl...@gmail.com>
>>> wrote:
>>>
>>> > I took a look at this with Mike a bit, and it seems like it's pretty
>>> > painful and without a clear way to avoid remerging conflicts. If the
>>> > latest attempt doesn't work, I'm in favor of getting it in and just
>>> getting
>>> > it down to as few commits as reasonably possible.
>>> >
>>> > On Thu, Nov 15, 2018 at 4:12 PM Michael Miklavcic <
>>> > michael.miklav...@gmail.com> wrote:
>>> >
>>> > > I'm attempting 1 more option, which would be to do a "git merge
>>> > > --strategy-option theirs" after having done the commit wrangling in
>>> the
>>> > PR
>>> > > branch. Will reply back with results.
>>> > >
>>> > > On Thu, Nov 15, 2018 at 2:02 PM Michael Miklavcic <
>>> > > michael.miklav...@gmail.com> wrote:
>>> > >
>>> > > > Yes, definitely.
>>> > > >
>>> > > > On Thu, Nov 15, 2018 at 2:01 PM Casey Stella <ceste...@gmail.com>
>>> > wrote:
>>> > > >
>>> > > >> Can you at least rename your commits to have METRON-1834 prefixing
>>> > them?
>>> > > >> On Thu, Nov 15, 2018 at 15:19 Michael Miklavcic <
>>> > > >> michael.miklav...@gmail.com>
>>> > > >> wrote:
>>> > > >>
>>> > > >> > https://github.com/apache/metron/pull/1242
>>> > > >> >
>>> > > >> > TL;DR
>>> > > >> > I'd like to discuss the best option to merge METRON-1834 into
>>> > master.
>>> > > I
>>> > > >> > want to propose handling this like a feature branch and merging
>>> it
>>> > > >> as-is.
>>> > > >> > ---
>>> > > >> >
>>> > > >> > I'm sure most folks' initial reaction will be some skepticism
>>> akin
>>> > to
>>> > > >> "have
>>> > > >> > you tried turning it off again," as this was my initial
>>> reaction as
>>> > > >> well.
>>> > > >> > It does not seem like this should be difficult. And I'm hoping
>>> that
>>> > > this
>>> > > >> > may be some esoteric thing on my system, though I believe this
>>> is a
>>> > > real
>>> > > >> > problem. A rather tedious explanation follows of what I've
>>> tried and
>>> > > the
>>> > > >> > problems encountered along the way. What seemed like a really
>>> simple
>>> > > >> > problem instead appears to be a bit much for Git to handle
>>> without
>>> > > >> > requiring redoing merges and another full round of testing. I'd
>>> much
>>> > > >> prefer
>>> > > >> > to avoid that in this instance.
>>> > > >> >
>>> > > >> > This PR is ready to be merged into master. It's recent and very
>>> > close
>>> > > to
>>> > > >> > fully up to date in the branch. Latest master merges cleanly.
>>> There
>>> > is
>>> > > >> an
>>> > > >> > attribution to Casey Stella for the base point of this PR that I
>>> > need
>>> > > to
>>> > > >> > include when getting this into master. When I created my
>>> branch, I
>>> > > >> > collapsed his initial set of commits into a single squashed
>>> commit
>>> > on
>>> > > >> > master at the time, and I started to work from there. Over
>>> time, I
>>> > > made
>>> > > >> a
>>> > > >> > number of additional commits and merges from master. Now for the
>>> > > issues.
>>> > > >> >
>>> > > >> > Originally, my expectation was that I could have 2 commits - the
>>> > > >> original
>>> > > >> > squashed commit from Casey along with all my additional commits
>>> (and
>>> > > the
>>> > > >> > merges with master) right on top. Nice clean history on master.
>>> > Turns
>>> > > >> out,
>>> > > >> > this doesn't work as cleanly as expected because a combination
>>> of
>>> > the
>>> > > >> > multiple merges and the need to keep the original commit with
>>> > > >> attribution
>>> > > >> > to Casey's work. A normal git pull --squash works fine, as
>>> expected,
>>> > > >> but we
>>> > > >> > lose the base commit, and therefore the requisite attribution.
>>> Here
>>> > > are
>>> > > >> > some other things I've tried, to no avail.
>>> > > >> >
>>> > > >> > 1. Git pull --squash after a merge with master. This will squash
>>> > > the
>>> > > >> > entire tree back to the branch point. No good.
>>> > > >> > 2. Git rebase -i master. Allows you to cleanly apply changes,
>>> but
>>> > > >> then
>>> > > >> > it ends up having problems with a clean rebase and shows
>>> > > conflicts. I
>>> > > >> > expect this is because of the merge history being necessary.
>>> > > >> > 3. Checking out a branch from the base point squashed commit
>>> from
>>> > > >> Casey,
>>> > > >> > and attempt to apply my changes on top. Numerous methods for
>>> > > >> > squashing/rebasing my changes on top applies nicely in the
>>> > branch.
>>> > > >> But
>>> > > >> > then
>>> > > >> > it once again causes merge conflicts when I attempt to get this
>>> > > onto
>>> > > >> > master. Things I attempted include: manually copying files,
>>> > > rebasing
>>> > > >> > all my
>>> > > >> > commits plus merges on top of the base commit, git merge
>>> > --squash,
>>> > > >> > intimidation.
>>> > > >> >
>>> > > >> > For one example of the result I'm talking about, this looks
>>> "good"
>>> > but
>>> > > >> it's
>>> > > >> > missing a ton of recent commits because they get caught up in
>>> the
>>> > > rebase
>>> > > >> > and get squashed in with my commit. When you attempt to merge
>>> this
>>> > > onto
>>> > > >> > master, it is just plain wrong (see example below with merge
>>> > > conflicts).
>>> > > >> > * 22c3b3bc 2018-11-15 | METRON-1834: Migrate Elasticsearch from
>>> > > >> > TransportClient to new Java REST API (mmiklavc via mmiklavc)
>>> closes
>>> > > >> > apache/metron#1242 (HEAD -> stella-es-base2) [mmiklavc]
>>> > > >> > * 84232e90 2018-10-08 | METRON-1834: Elasticsearch rest client
>>> > > migration
>>> > > >> > base work starting point for apache/metron#1242 (cstella via
>>> > mmiklavc)
>>> > > >> > [cstella]
>>> > > >> > * 5bfc08c5 2018-10-08 | METRON-1792 Simplify Profile
>>> Definitions in
>>> > > >> > Integration Tests (nickwallen) closes apache/metron#1211
>>> > [nickwallen]
>>> > > >> >
>>> > > >> > Here's 1 merge conflict (say what??)
>>> > > >> > CONFLICT (rename/rename): Rename
>>> > > >> >
>>> > > >> >
>>> > > >>
>>> > >
>>> >
>>> "metron-interface/metron-config/src/app/rxjs-operators.ts"->"metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/ParserRunnerResults.java"
>>> > > >> > in branch "HEAD" rename
>>> > > >> >
>>> > > >> >
>>> > > >>
>>> > >
>>> >
>>> "metron-interface/metron-config/src/app/rxjs-operators.ts"->"metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/utils/FieldMapping.java"
>>> > > >> > in "stella-es-base2"
>>> > > >> >
>>> > > >> > If I attempt to use rebase on master instead of merge, it really
>>> > seems
>>> > > >> to
>>> > > >> > mess up the files. Again, another example where I have TODO's
>>> that
>>> > are
>>> > > >> most
>>> > > >> > definitely removed by a commit in my branch and also do not
>>> exist in
>>> > > >> > master. I'm not sure what's happening here, but I don't trust
>>> it.
>>> > > >> > {
>>> > > >> > //TODO: It shouldn't require an assertEventually() here as it
>>> > > >> should
>>> > > >> > be synchronous.
>>> > > >> > // Before merging, please figure out why.
>>> > > >> > TestUtils.assertEventually(() -> Assert.assertEquals(14,
>>> > > >> >
>>> dao.getColumnMetadata(Collections.singletonList("snort")).size()));
>>> > > >> > Map<String, FieldType> fieldTypes =
>>> > > >> > dao.getColumnMetadata(Collections.singletonList("snort"));
>>> > > >> > <<<<<<< HEAD
>>> > > >> > Assert.assertEquals(32, fieldTypes.size());
>>> > > >> > Assert.assertEquals(FieldType.KEYWORD,
>>> > > >> > fieldTypes.get("sig_generator"));
>>> > > >> > =======
>>> > > >> > Assert.assertEquals(FieldType.INTEGER,
>>> > > >> > fieldTypes.get("snort_field"));
>>> > > >> > >>>>>>> METRON-1834: Elasticsearch rest client migration base
>>> work
>>> > > >> starting
>>> > > >> > point for apache/metron#1242 (cstella via mmiklavc)
>>> > > >> >
>>> > > >>
>>> > > >
>>> > >
>>> >
>>>
>>>
